memory oh baby

firewall/config.go

@@ -19,19 +19,19 @@ type ruleAdder interface {
 	AddRule(incoming bool, proto uint8, startPort int32, endPort int32, groups []string, host string, cidr, localCidr string, caName string, caSha string) error
 }
 
-// RulesFromConfig reads the inbound and outbound firewall rules from c
-func RulesFromConfig(l *slog.Logger, c *config.C) (*RulesBuilder, error) {
+// RulesFromConfig reads the inbound and outbound firewall rules from c and builds them, see RulesBuilder.Build
+func RulesFromConfig(l *slog.Logger, c *config.C, vpnNetworks, unsafeNetworks []netip.Prefix) (Rules, error) {
 	b := NewRulesBuilder(l)
 	b.defaultLocalCIDRAny = c.GetBool("firewall.default_local_cidr_any", false)
 
 	if err := addRulesFromConfig(l, false, c, b); err != nil {
-		return nil, err
+		return Rules{}, err
 	}
 	if err := addRulesFromConfig(l, true, c, b); err != nil {
-		return nil, err
+		return Rules{}, err
 	}
 
-	return b, nil
+	return b.Build(vpnNetworks, unsafeNetworks), nil
 }
 
 func addRulesFromConfig(l *slog.Logger, inbound bool, c *config.C, ra ruleAdder) error {
@@ -114,20 +114,6 @@ func addRulesFromConfig(l *slog.Logger, inbound bool, c *config.C, ra ruleAdder)
 			return fmt.Errorf("%s rule #%v; %s %s", table, i, errPort, err)
 		}
 
-		if r.Cidr != "" && r.Cidr != "any" {
-			_, err = netip.ParsePrefix(r.Cidr)
-			if err != nil {
-				return fmt.Errorf("%s rule #%v; cidr did not parse; %s", table, i, err)
-			}
-		}
-
-		if r.LocalCidr != "" && r.LocalCidr != "any" {
-			_, err = netip.ParsePrefix(r.LocalCidr)
-			if err != nil {
-				return fmt.Errorf("%s rule #%v; local_cidr did not parse; %s", table, i, err)
-			}
-		}
-
 		if warning := r.sanity(); warning != nil {
 			l.Warn("firewall rule sanity check",
 				"table", table,
@@ -138,7 +124,7 @@ func addRulesFromConfig(l *slog.Logger, inbound bool, c *config.C, ra ruleAdder)
 
 		err = ra.AddRule(inbound, proto, startPort, endPort, r.Groups, r.Host, r.Cidr, r.LocalCidr, r.CAName, r.CASha)
 		if err != nil {
-			return fmt.Errorf("%s rule #%v; `%s`", table, i, err)
+			return fmt.Errorf("%s rule #%v; %s", table, i, err)
 		}
 	}
 

firewall/config_test.go

@@ -224,11 +224,10 @@ 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: nil, ip: "any", localIp: ""}, mf.lastCall)
 
-	// Test adding rule with junk cidr
+	// Test adding rule with junk cidr, which RulesBuilder rejects
 	conf = config.NewC(test.NewLogger())
-	mf = &mockFirewall{}
 	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "cidr": "junk/junk"}}}
-	require.EqualError(t, addRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; cidr did not parse; netip.ParsePrefix(\"junk/junk\"): ParseAddr(\"junk\"): unable to parse IP")
+	require.EqualError(t, addRulesFromConfig(l, true, conf, NewRulesBuilder(l)), "firewall.inbound rule #0; cidr did not parse; netip.ParsePrefix(\"junk/junk\"): ParseAddr(\"junk\"): unable to parse IP")
 
 	// Test adding rule with local_cidr ipv6
 	conf = config.NewC(test.NewLogger())
@@ -244,11 +243,10 @@ 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: nil, localIp: "any"}, mf.lastCall)
 
-	// Test adding rule with junk local_cidr
+	// Test adding rule with junk local_cidr, which RulesBuilder rejects
 	conf = config.NewC(test.NewLogger())
-	mf = &mockFirewall{}
 	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "local_cidr": "junk/junk"}}}
-	require.EqualError(t, addRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; local_cidr did not parse; netip.ParsePrefix(\"junk/junk\"): ParseAddr(\"junk\"): unable to parse IP")
+	require.EqualError(t, addRulesFromConfig(l, true, conf, NewRulesBuilder(l)), "firewall.inbound rule #0; local_cidr did not parse; netip.ParsePrefix(\"junk/junk\"): ParseAddr(\"junk\"): unable to parse IP")
 
 	// Test adding rule with ca_sha
 	conf = config.NewC(test.NewLogger())
@@ -290,7 +288,63 @@ func TestAddRulesFromConfig(t *testing.T) {
 	mf = &mockFirewall{}
 	mf.nextCallReturn = errors.New("test error")
 	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "host": "a"}}}
-	require.EqualError(t, addRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; `test error`")
+	require.EqualError(t, addRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; test error")
+}
+
+// TestRulesFromConfig_defaultLocalCIDR ensures rules without a local_cidr allow only the vpn networks when there are
+// unsafe networks, unless default_local_cidr_any is set, and that a rule's own local_cidr is kept either way
+func TestRulesFromConfig_defaultLocalCIDR(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")
+
+	allows := func(lr *localRules, addr netip.Addr) bool {
+		return lr.match(&Packet{LocalAddr: addr}, nil)
+	}
+
+	for _, tc := range []struct {
+		name                string
+		unsafeNetworks      []netip.Prefix
+		defaultLocalCIDRAny bool
+		wantUnsafe          bool
+	}{
+		{"no unsafe networks", nil, false, true},
+		{"unsafe networks", unsafeNetworks, false, false},
+		{"unsafe networks and default_local_cidr_any", unsafeNetworks, true, true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			conf := config.NewC(test.NewLogger())
+			conf.Settings["firewall"] = map[string]any{
+				"default_local_cidr_any": tc.defaultLocalCIDRAny,
+				"inbound": []any{
+					map[string]any{"port": "any", "proto": "any", "host": "any"},
+					map[string]any{"port": "22", "proto": "tcp", "host": "any"},
+					map[string]any{"port": "80", "proto": "tcp", "host": "any", "local_cidr": "192.168.0.0/24"},
+					map[string]any{"port": "443", "proto": "tcp", "host": "any", "local_cidr": "any"},
+				},
+			}
+			rules, err := RulesFromConfig(test.NewLogger(), conf, vpnNetworks, tc.unsafeNetworks)
+			require.NoError(t, err)
+
+			// The proto `any` rule, copied into a protocol with rules of its own and shared by one without
+			for _, proto := range []uint8{iputil.IPProtocolTCP, 47} {
+				lr := rules.In.protos[proto][PortAny].Any.Any
+				assert.True(t, allows(lr, vpnAddr), "proto %d", proto)
+				assert.Equal(t, tc.wantUnsafe, allows(lr, unsafeAddr), "proto %d", proto)
+			}
+
+			tcp := rules.In.protos[iputil.IPProtocolTCP]
+			assert.True(t, allows(tcp[22].Any.Any, vpnAddr))
+			assert.Equal(t, tc.wantUnsafe, allows(tcp[22].Any.Any, unsafeAddr))
+
+			assert.False(t, allows(tcp[80].Any.Any, vpnAddr))
+			assert.True(t, allows(tcp[80].Any.Any, unsafeAddr))
+
+			assert.True(t, allows(tcp[443].Any.Any, vpnAddr))
+			assert.True(t, allows(tcp[443].Any.Any, unsafeAddr))
+		})
+	}
 }
 
 func Test_convertRule(t *testing.T) {

firewall/rules_builder.go

@@ -29,16 +29,6 @@ type RulesBuilder struct {
 	l *slog.Logger
 }
 
-// Rules are the Tables built for each direction, with hashes of the rules they were built from
-type Rules struct {
-	In, Out *Table
-
-	// Hash is a sha256 of the rules, HashFNV is an FNV-1a of them for use as a metric value.
-	// Rule sets with the same rules have the same hashes.
-	Hash    string
-	HashFNV uint32
-}
-
 // tableRules are one direction's rules, waiting to be built into a Table
 type tableRules struct {
 	anyRules []rule
@@ -140,13 +130,13 @@ func parseRule(startPort, endPort int32, groups []string, host, cidr, localCidr,
 	var err error
 	r.cidr, r.anyCIDR, err = parseCIDR(cidr)
 	if err != nil {
-		return rule{}, err
+		return rule{}, fmt.Errorf("cidr did not parse; %w", err)
 	}
 
 	var localCIDR netip.Prefix
 	localCIDR, r.anyLocalCIDR, err = parseCIDR(localCidr)
 	if err != nil {
-		return rule{}, err
+		return rule{}, fmt.Errorf("local_cidr did not parse; %w", err)
 	}
 	if localCIDR.IsValid() {
 		r.localCIDRs = []netip.Prefix{localCIDR}
@@ -186,19 +176,19 @@ func (tr *tableRules) build(defaultLocalAny bool, defaultLocalCIDRs []netip.Pref
 	}
 
 	t := &Table{}
-	for proto := range t.Protos {
+	for proto := range t.protos {
 		rules := tr.protoRules[proto]
 		if len(rules) == 0 {
-			t.Protos[proto] = anyProto
+			t.protos[proto] = anyProto
 			continue
 		}
 
 		pr := portRules{}
 		add(pr, tr.anyRules)
 		add(pr, rules)
-		t.Protos[proto] = pr
+		t.protos[proto] = pr
 	}
-	t.Protos[iputil.IPProtocolICMPv6] = t.Protos[iputil.IPProtocolICMP]
+	t.protos[iputil.IPProtocolICMPv6] = t.protos[iputil.IPProtocolICMP]
 
 	return t
 }
@@ -227,70 +217,45 @@ func (r rule) isAny() bool {
 
 func (pr portRules) add(r rule) {
 	for i := r.startPort; i <= r.endPort; i++ {
-		if _, ok := pr[i]; !ok {
-			pr[i] = &caRules{
-				CANames: make(map[string]*remoteRules),
-				CAShas:  make(map[string]*remoteRules),
-			}
+		cr := pr[i]
+		if cr == nil {
+			cr = &caRules{}
+			pr[i] = cr
 		}
-
-		pr[i].add(r)
+		cr.add(r)
 	}
 }
 
 func (cr *caRules) add(r rule) {
-	newRemoteRules := func() *remoteRules {
-		return &remoteRules{
-			Hosts:  make(map[string]*localRules),
-			Groups: make([]*groupsRule, 0),
-			CIDR:   new(bart.Table[*localRules]),
-		}
-	}
-
 	if r.caSha == "" && r.caName == "" {
 		if cr.Any == nil {
-			cr.Any = newRemoteRules()
+			cr.Any = &remoteRules{}
 		}
-
 		cr.Any.add(r)
 		return
 	}
 
 	if r.caSha != "" {
-		if _, ok := cr.CAShas[r.caSha]; !ok {
-			cr.CAShas[r.caSha] = newRemoteRules()
-		}
-		cr.CAShas[r.caSha].add(r)
+		getOrNew(&cr.CAShas, r.caSha).add(r)
 	}
 
 	if r.caName != "" {
-		if _, ok := cr.CANames[r.caName]; !ok {
-			cr.CANames[r.caName] = newRemoteRules()
-		}
-		cr.CANames[r.caName].add(r)
+		getOrNew(&cr.CANames, r.caName).add(r)
 	}
 }
 
 func (rr *remoteRules) add(r rule) {
-	newLocalRules := func() *localRules {
-		return &localRules{
-			LocalCIDR: new(bart.Lite),
-		}
-	}
-
 	if r.isAny() {
 		if rr.Any == nil {
-			rr.Any = newLocalRules()
+			rr.Any = &localRules{}
 		}
-
 		rr.Any.add(r)
 		return
 	}
 
 	if len(r.groups) > 0 {
-		lr := newLocalRules()
+		lr := &localRules{}
 		lr.add(r)
-
 		rr.Groups = append(rr.Groups, &groupsRule{
 			Groups:    r.groups,
 			LocalCIDR: lr,
@@ -298,21 +263,19 @@ func (rr *remoteRules) add(r rule) {
 	}
 
 	if r.host != "" {
-		lr := rr.Hosts[r.host]
-		if lr == nil {
-			lr = newLocalRules()
-		}
-		lr.add(r)
-		rr.Hosts[r.host] = lr
+		getOrNew(&rr.Hosts, r.host).add(r)
 	}
 
 	if r.cidr.IsValid() {
-		lr, _ := rr.CIDR.Get(r.cidr)
-		if lr == nil {
-			lr = newLocalRules()
+		if rr.CIDR == nil {
+			rr.CIDR = new(bart.Table[*localRules])
+		}
+		lr, ok := rr.CIDR.Get(r.cidr)
+		if !ok {
+			lr = &localRules{}
+			rr.CIDR.Insert(r.cidr, lr)
 		}
 		lr.add(r)
-		rr.CIDR.Insert(r.cidr, lr)
 	}
 }
 
@@ -322,7 +285,23 @@ func (lr *localRules) add(r rule) {
 		return
 	}
 
+	if lr.LocalCIDR == nil {
+		lr.LocalCIDR = new(bart.Lite)
+	}
 	for _, prefix := range r.localCIDRs {
 		lr.LocalCIDR.Insert(prefix)
 	}
 }
+
+// getOrNew returns m[k], adding a new value for k first when there isn't one. m is made if it's nil.
+func getOrNew[M ~map[K]*V, K comparable, V any](m *M, k K) *V {
+	if *m == nil {
+		*m = make(M)
+	}
+	v := (*m)[k]
+	if v == nil {
+		v = new(V)
+		(*m)[k] = v
+	}
+	return v
+}

firewall/table.go

@@ -8,21 +8,33 @@ import (
 	"github.com/slackhq/nebula/iputil"
 )
 
+// Rules are the Tables built for each direction, with hashes of the rules they were built from
+type Rules struct {
+	In, Out *Table
+
+	// Hash is a sha256 of the rules, HashFNV is an FNV-1a of them for use as a metric value.
+	// Rule sets with the same rules have the same hashes.
+	Hash    string
+	HashFNV uint32
+}
+
 // Table holds the rules for one direction, the evaluation order is:
 // Proto AND port AND (CA SHA or CA name) AND local CIDR AND (group OR groups OR name OR remote CIDR)
 // A Table doesn't change once built, see RulesBuilder.
 type Table struct {
-	// Protos holds the rules for each IP protocol number, so a packet is only checked against one set of rules.
+	// protos holds the rules for each IP protocol number, so a packet is only checked against one set of rules.
 	// A protocol with rules of its own also holds a copy of every proto `any` rule, the rest share the proto `any`
-	// rules, which are at Protos[ProtoAny]. nil when a protocol has no rules at all.
+	// rules, which are at protos[ProtoAny]. nil when a protocol has no rules at all.
 	// ICMP and ICMPv6 share one set of rules.
-	Protos [256]portRules
+	protos [256]portRules
 }
 
 // Even though ports are uint16, int32 maps are faster for lookup
 // Plus we can use `-1` for fragment rules
 type portRules map[int32]*caRules
 
+// caRules and the types below it only make their maps and tables once a rule needs them, a range of ports
+// gets a copy for every port so empty ones add up
 type caRules struct {
 	Any     *remoteRules
 	CANames map[string]*remoteRules
@@ -43,13 +55,23 @@ type groupsRule struct {
 }
 
 type localRules struct {
-	Any       bool
+	Any bool
+	// LocalCIDR is always set when Any isn't
 	LocalCIDR *bart.Lite
 }
 
+// Match reports whether a rule for the direction p is going allows it
+func (r *Rules) Match(p *Packet, incoming bool, c *cert.CachedCertificate, caPool *cert.CAPool) bool {
+	t := r.Out
+	if incoming {
+		t = r.In
+	}
+	return t.Match(p, incoming, c, caPool)
+}
+
 // Match reports whether a rule allows p
 func (t *Table) Match(p *Packet, incoming bool, c *cert.CachedCertificate, caPool *cert.CAPool) bool {
-	return t.Protos[p.Protocol].match(p, incoming, c, caPool)
+	return t.protos[p.Protocol].match(p, incoming, c, caPool)
 }
 
 func (pr portRules) match(p *Packet, incoming bool, c *cert.CachedCertificate, caPool *cert.CAPool) bool {
@@ -142,9 +164,11 @@ func (rr *remoteRules) match(p *Packet, c *cert.CachedCertificate) bool {
 		}
 	}
 
-	for _, v := range rr.CIDR.Supernets(netip.PrefixFrom(p.RemoteAddr, p.RemoteAddr.BitLen())) {
-		if v.match(p, c) {
-			return true
+	if rr.CIDR != nil {
+		for _, v := range rr.CIDR.Supernets(netip.PrefixFrom(p.RemoteAddr, p.RemoteAddr.BitLen())) {
+			if v.match(p, c) {
+				return true
+			}
 		}
 	}