diff --git a/firewall/rules_builder.go b/firewall/rules_builder.go index 2f08c170..4b6eca9c 100644 --- a/firewall/rules_builder.go +++ b/firewall/rules_builder.go @@ -47,9 +47,10 @@ type rule struct { cidr netip.Prefix anyCIDR bool - // localCIDRs are the local addresses the rule allows, anyLocalCIDR allows all of them. + // localCIDR is the local addresses the rule allows, anyLocalCIDR allows all of them. // Neither is set for a rule without a local cidr until the Table is built and it gets the default. - localCIDRs []netip.Prefix + // localCIDR is shared by every localRules the rule is added to, so it's never changed, see localRules.add + localCIDR *bart.Lite anyLocalCIDR bool } @@ -104,10 +105,17 @@ func (b *RulesBuilder) AddRule(incoming bool, proto uint8, startPort int32, endP // Build turns the rules into a Table for each direction, and hashes them. Rules without a local cidr allow // vpnNetworks when there are unsafe networks, otherwise any local address. func (b *RulesBuilder) Build(vpnNetworks, unsafeNetworks []netip.Prefix) Rules { - anyLocal := len(unsafeNetworks) == 0 || b.defaultLocalCIDRAny + var defaultLocal *bart.Lite + if len(unsafeNetworks) > 0 && !b.defaultLocalCIDRAny { + defaultLocal = new(bart.Lite) + for _, network := range vpnNetworks { + defaultLocal.Insert(network) + } + } + return Rules{ - In: b.in.build(anyLocal, vpnNetworks), - Out: b.out.build(anyLocal, vpnNetworks), + In: b.in.build(defaultLocal), + Out: b.out.build(defaultLocal), Hash: hex.EncodeToString(b.ruleSHA.Sum(nil)), HashFNV: b.ruleFNV.Sum32(), } @@ -139,7 +147,8 @@ func parseRule(startPort, endPort int32, groups []string, host, cidr, localCidr, return rule{}, fmt.Errorf("local_cidr did not parse; %w", err) } if localCIDR.IsValid() { - r.localCIDRs = []netip.Prefix{localCIDR} + r.localCIDR = new(bart.Lite) + r.localCIDR.Insert(localCIDR) } return r, nil @@ -157,42 +166,44 @@ func (tr *tableRules) add(proto uint8, r rule) { } } -// build turns the rules into a Table. Rules without a local cidr allow defaultLocalCIDRs, -// or any local address when defaultLocalAny is set. -func (tr *tableRules) build(defaultLocalAny bool, defaultLocalCIDRs []netip.Prefix) *Table { - add := func(pr portRules, rules []rule) { - for _, r := range rules { - if !r.anyLocalCIDR && r.localCIDRs == nil { - r.anyLocalCIDR, r.localCIDRs = defaultLocalAny, defaultLocalCIDRs - } - pr.add(r) - } - } - - var anyProto portRules - if len(tr.anyRules) > 0 { - anyProto = portRules{} - add(anyProto, tr.anyRules) - } - +// build turns the rules into a Table. Rules without a local cidr allow defaultLocal, +// or any local address when defaultLocal is nil. +func (tr *tableRules) build(defaultLocal *bart.Lite) *Table { t := &Table{} - for proto := range t.protos { - rules := tr.protoRules[proto] - if len(rules) == 0 { - t.protos[proto] = anyProto - continue - } - pr := portRules{} - add(pr, tr.anyRules) - add(pr, rules) - t.protos[proto] = pr + // Every protocol starts out sharing the proto `any` rules, including protos[ProtoAny] + anyProto := newPortRules(defaultLocal, tr.anyRules) + for proto := range t.protos { + t.protos[proto] = anyProto } + + // Protocols with rules of their own get a copy of the proto `any` rules too, so a packet only checks one set + for proto, rules := range tr.protoRules { + if len(rules) > 0 { + t.protos[proto] = newPortRules(defaultLocal, tr.anyRules, rules) + } + } + + // ICMP and ICMPv6 share one set of rules, add keeps them together under ICMP t.protos[iputil.IPProtocolICMPv6] = t.protos[iputil.IPProtocolICMP] return t } +// newPortRules builds portRules from each set of rules, or returns nil when there are none +func newPortRules(defaultLocal *bart.Lite, ruleSets ...[]rule) portRules { + var pr portRules + for _, rules := range ruleSets { + for _, r := range rules { + if pr == nil { + pr = portRules{} + } + pr.add(r.withDefaultLocal(defaultLocal)) + } + } + return pr +} + // parseCIDR parses a rule's cidr, which is empty, `any`, or a prefix func parseCIDR(s string) (prefix netip.Prefix, isAny bool, err error) { switch s { @@ -215,6 +226,16 @@ func (r rule) isAny() bool { return slices.Contains(r.groups, "any") || r.host == "any" || r.anyCIDR } +// withDefaultLocal returns r allowing defaultLocal, or any local address when defaultLocal is nil, +// unless r has a local cidr of its own +func (r rule) withDefaultLocal(defaultLocal *bart.Lite) rule { + if !r.anyLocalCIDR && r.localCIDR == nil { + r.anyLocalCIDR = defaultLocal == nil + r.localCIDR = defaultLocal + } + return r +} + func (pr portRules) add(r rule) { for i := r.startPort; i <= r.endPort; i++ { cr := pr[i] @@ -285,11 +306,13 @@ func (lr *localRules) add(r rule) { return } + // Share the rule's local cidrs until another rule's need adding, then copy them instead of changing them if lr.LocalCIDR == nil { - lr.LocalCIDR = new(bart.Lite) + lr.LocalCIDR = r.localCIDR + return } - for _, prefix := range r.localCIDRs { - lr.LocalCIDR.Insert(prefix) + for prefix := range r.localCIDR.All() { + lr.LocalCIDR = lr.LocalCIDR.InsertPersist(prefix) } } diff --git a/firewall/rules_builder_test.go b/firewall/rules_builder_test.go new file mode 100644 index 00000000..92c4bcaa --- /dev/null +++ b/firewall/rules_builder_test.go @@ -0,0 +1,220 @@ +package firewall + +import ( + "math" + "net/netip" + "testing" + + "github.com/slackhq/nebula/iputil" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRulesBuilder_AddRule(t *testing.T) { + l := test.NewLogger() + + ti, err := netip.ParsePrefix("1.2.3.4/32") + require.NoError(t, err) + + ti6, err := netip.ParsePrefix("fd12::34/128") + require.NoError(t, err) + + rb := NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, 1, 1, []string{}, "", "", "", "", "")) + rules := rb.Build(nil, nil) + // An empty rule is any + assert.True(t, rules.In.protos[iputil.IPProtocolTCP][1].Any.Any.Any) + assert.Empty(t, rules.In.protos[iputil.IPProtocolTCP][1].Any.Groups) + assert.Empty(t, rules.In.protos[iputil.IPProtocolTCP][1].Any.Hosts) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolUDP, 1, 1, []string{"g1"}, "", "", "", "", "")) + rules = rb.Build(nil, nil) + assert.Nil(t, rules.In.protos[iputil.IPProtocolUDP][1].Any.Any) + assert.Contains(t, rules.In.protos[iputil.IPProtocolUDP][1].Any.Groups[0].Groups, "g1") + assert.Empty(t, rules.In.protos[iputil.IPProtocolUDP][1].Any.Hosts) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolICMP, 1, 1, []string{}, "h1", "", "", "", "")) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolICMPv6, 1, 1, []string{}, "h2", "", "", "", "")) + rules = rb.Build(nil, nil) + //no matter what port is given for icmp, it should end up as "any" + assert.Nil(t, rules.In.protos[iputil.IPProtocolICMP][PortAny].Any.Any) + assert.Empty(t, rules.In.protos[iputil.IPProtocolICMP][PortAny].Any.Groups) + assert.Contains(t, rules.In.protos[iputil.IPProtocolICMP][PortAny].Any.Hosts, "h1") + + // ICMP and ICMPv6 share one set of rules + assert.Contains(t, rules.In.protos[iputil.IPProtocolICMP][PortAny].Any.Hosts, "h2") + assert.Contains(t, rules.In.protos[iputil.IPProtocolICMPv6][PortAny].Any.Hosts, "h1") + + // Any other protocol number gets its own rules + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, 47, 0, 0, []string{"g1"}, "", "", "", "", "")) // GRE + rules = rb.Build(nil, nil) + assert.Contains(t, rules.In.protos[47][PortAny].Any.Groups[0].Groups, "g1") + assert.Nil(t, rules.In.protos[iputil.IPProtocolTCP]) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 1, 1, []string{}, "", ti.String(), "", "", "")) + rules = rb.Build(nil, nil) + assert.Nil(t, rules.Out.protos[ProtoAny][1].Any.Any) + _, ok := rules.Out.protos[ProtoAny][1].Any.CIDR.Get(ti) + assert.True(t, ok) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 1, 1, []string{}, "", ti6.String(), "", "", "")) + rules = rb.Build(nil, nil) + assert.Nil(t, rules.Out.protos[ProtoAny][1].Any.Any) + _, ok = rules.Out.protos[ProtoAny][1].Any.CIDR.Get(ti6) + assert.True(t, ok) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 1, 1, []string{}, "", "", ti.String(), "", "")) + rules = rb.Build(nil, nil) + assert.NotNil(t, rules.Out.protos[ProtoAny][1].Any.Any) + ok = rules.Out.protos[ProtoAny][1].Any.Any.LocalCIDR.Get(ti) + assert.True(t, ok) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 1, 1, []string{}, "", "", ti6.String(), "", "")) + rules = rb.Build(nil, nil) + assert.NotNil(t, rules.Out.protos[ProtoAny][1].Any.Any) + ok = rules.Out.protos[ProtoAny][1].Any.Any.LocalCIDR.Get(ti6) + assert.True(t, ok) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolUDP, 1, 1, []string{"g1"}, "", "", "", "ca-name", "")) + rules = rb.Build(nil, nil) + assert.Contains(t, rules.In.protos[iputil.IPProtocolUDP][1].CANames, "ca-name") + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolUDP, 1, 1, []string{"g1"}, "", "", "", "", "ca-sha")) + rules = rb.Build(nil, nil) + assert.Contains(t, rules.In.protos[iputil.IPProtocolUDP][1].CAShas, "ca-sha") + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "any", "", "", "", "")) + rules = rb.Build(nil, nil) + assert.True(t, rules.Out.protos[ProtoAny][0].Any.Any.Any) + + rb = NewRulesBuilder(l) + anyIp, err := netip.ParsePrefix("0.0.0.0/0") + require.NoError(t, err) + + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "", anyIp.String(), "", "", "")) + rules = rb.Build(nil, nil) + assert.Nil(t, rules.Out.protos[ProtoAny][0].Any.Any) + table, ok := rules.Out.protos[ProtoAny][0].Any.CIDR.Lookup(netip.MustParseAddr("1.1.1.1")) + assert.True(t, table.Any) + table, ok = rules.Out.protos[ProtoAny][0].Any.CIDR.Lookup(netip.MustParseAddr("9::9")) + assert.False(t, ok) + + rb = NewRulesBuilder(l) + anyIp6, err := netip.ParsePrefix("::/0") + require.NoError(t, err) + + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "", anyIp6.String(), "", "", "")) + rules = rb.Build(nil, nil) + assert.Nil(t, rules.Out.protos[ProtoAny][0].Any.Any) + table, ok = rules.Out.protos[ProtoAny][0].Any.CIDR.Lookup(netip.MustParseAddr("9::9")) + assert.True(t, table.Any) + table, ok = rules.Out.protos[ProtoAny][0].Any.CIDR.Lookup(netip.MustParseAddr("1.1.1.1")) + assert.False(t, ok) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "", "any", "", "", "")) + rules = rb.Build(nil, nil) + assert.True(t, rules.Out.protos[ProtoAny][0].Any.Any.Any) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "", "", anyIp.String(), "", "")) + rules = rb.Build(nil, nil) + assert.False(t, rules.Out.protos[ProtoAny][0].Any.Any.Any) + assert.True(t, rules.Out.protos[ProtoAny][0].Any.Any.LocalCIDR.Lookup(netip.MustParseAddr("1.1.1.1"))) + assert.False(t, rules.Out.protos[ProtoAny][0].Any.Any.LocalCIDR.Lookup(netip.MustParseAddr("9::9"))) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "", "", anyIp6.String(), "", "")) + rules = rb.Build(nil, nil) + assert.False(t, rules.Out.protos[ProtoAny][0].Any.Any.Any) + assert.True(t, rules.Out.protos[ProtoAny][0].Any.Any.LocalCIDR.Lookup(netip.MustParseAddr("9::9"))) + assert.False(t, rules.Out.protos[ProtoAny][0].Any.Any.LocalCIDR.Lookup(netip.MustParseAddr("1.1.1.1"))) + + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(false, ProtoAny, 0, 0, []string{}, "", "", "any", "", "")) + rules = rb.Build(nil, nil) + assert.True(t, rules.Out.protos[ProtoAny][0].Any.Any.Any) + + // Every protocol number is accepted + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, math.MaxUint8, 0, 0, []string{}, "", "", "", "", "")) + rules = rb.Build(nil, nil) + assert.True(t, rules.In.protos[math.MaxUint8][PortAny].Any.Any.Any) + + // Test error conditions, a bad rule is reported when it's added + rb = NewRulesBuilder(l) + require.Error(t, rb.AddRule(true, ProtoAny, 10, 0, []string{}, "", "", "", "", "")) + require.Error(t, rb.AddRule(true, ProtoAny, 0, 0, []string{}, "", "junk", "", "", "")) + require.Error(t, rb.AddRule(true, ProtoAny, 0, 0, []string{}, "", "", "junk", "", "")) + + // ICMP and ICMPv6 share one copy of each proto `any` rule + rb = NewRulesBuilder(l) + require.NoError(t, rb.AddRule(true, ProtoAny, PortAny, PortAny, []string{"default-group"}, "", "", "", "", "")) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolICMPv6, PortAny, PortAny, []string{"nope"}, "", "", "", "", "")) + rules = rb.Build(nil, nil) + assert.Len(t, rules.In.protos[iputil.IPProtocolICMP][PortAny].Any.Groups, 2) + assert.Len(t, rules.In.protos[iputil.IPProtocolICMPv6][PortAny].Any.Groups, 2) + + // No rules allow nothing + rules = NewRulesBuilder(l).Build(nil, nil) + for proto := range rules.In.protos { + assert.Nil(t, rules.In.protos[proto], "proto %d", proto) + assert.Nil(t, rules.Out.protos[proto], "proto %d", proto) + } +} + +// TestRulesBuilder_sharedLocalCIDR ensures local cidrs shared between rules and ports are copied, not changed, +// when another rule's local cidrs are added to them +func TestRulesBuilder_sharedLocalCIDR(t *testing.T) { + vpnNetworks := []netip.Prefix{netip.MustParsePrefix("10.1.0.5/16")} + unsafeNetworks := []netip.Prefix{netip.MustParsePrefix("192.168.0.0/24")} + vpnAddr := netip.MustParseAddr("10.1.9.9") + unsafeAddr := netip.MustParseAddr("192.168.0.3") + otherAddr := netip.MustParseAddr("172.16.0.1") + + rb := NewRulesBuilder(test.NewLogger()) + // h1 allows the vpn networks by default on ports 1 and 2, and 172.16.0.0/12 as well on port 1 + require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, 1, 2, nil, "h1", "", "", "", "")) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, 1, 1, nil, "h1", "", "172.16.0.0/12", "", "")) + // h2 allows 192.168.0.0/24 on ports 1 and 2, and the vpn networks by default as well on port 1 + require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, 1, 2, nil, "h2", "", "192.168.0.0/24", "", "")) + require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, 1, 1, nil, "h2", "", "", "", "")) + // h3 only has the default + require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, 3, 3, nil, "h3", "", "", "", "")) + rules := rb.Build(vpnNetworks, unsafeNetworks) + + for _, tc := range []struct { + port int32 + host string + addr netip.Addr + want bool + }{ + {1, "h1", vpnAddr, true}, + {1, "h1", otherAddr, true}, + {1, "h1", unsafeAddr, false}, + {2, "h1", vpnAddr, true}, + {2, "h1", otherAddr, false}, + {1, "h2", unsafeAddr, true}, + {1, "h2", vpnAddr, true}, + {1, "h2", otherAddr, false}, + {2, "h2", unsafeAddr, true}, + {2, "h2", vpnAddr, false}, + {3, "h3", vpnAddr, true}, + {3, "h3", otherAddr, false}, + {3, "h3", unsafeAddr, false}, + } { + lr := rules.In.protos[iputil.IPProtocolTCP][tc.port].Any.Hosts[tc.host] + assert.Equal(t, tc.want, lr.match(&Packet{LocalAddr: tc.addr}, nil), "port %d, %s, %s", tc.port, tc.host, tc.addr) + } +} diff --git a/firewall/table.go b/firewall/table.go index 23631763..5827c90d 100644 --- a/firewall/table.go +++ b/firewall/table.go @@ -56,7 +56,7 @@ type groupsRule struct { type localRules struct { Any bool - // LocalCIDR is always set when Any isn't + // LocalCIDR is always set when Any isn't. It's shared with other localRules, so it's never changed. LocalCIDR *bart.Lite }