checkpt

firewall/config.go

@@ -84,13 +84,10 @@ func addRulesFromConfig(l *slog.Logger, inbound bool, c *config.C, ra ruleAdder)
 			}
 			proto = uint8(n)
 		}
-		// ICMPv6 rules are ICMP rules, see RulesBuilder.AddRule
-		if proto == iputil.IPProtocolICMPv6 {
-			proto = iputil.IPProtocolICMP
-		}
-
 		var startPort, endPort int32
-		if proto == iputil.IPProtocolICMP {
+		if proto == iputil.IPProtocolICMP || proto == iputil.IPProtocolICMPv6 {
+			// RulesBuilder.AddRule coerces ICMP ports to `any` as well. This copy stays so that an ICMP rule's
+			// port is warned about rather than parsed, whatever it holds.
 			startPort = PortAny
 			endPort = PortAny
 			if sPort != "" {

firewall/config_test.go

@@ -170,13 +170,21 @@ func TestAddRulesFromConfig(t *testing.T) {
 		assert.Equal(t, addRuleCall{incoming: false, proto: 47, startPort: PortAny, endPort: PortAny, groups: nil, host: "a", ip: "", localIp: ""}, mf.lastCall, proto)
 	}
 
-	// Test ICMP and ICMPv6, by name and by number, still ignore ports. An ICMPv6 rule is added as an ICMP rule.
-	for _, proto := range []any{"icmp", "icmpv6", iputil.IPProtocolICMP, iputil.IPProtocolICMPv6} {
+	// Test ICMP and ICMPv6, by name and by number, still ignore ports
+	for _, tc := range []struct {
+		given any
+		want  uint8
+	}{
+		{"icmp", iputil.IPProtocolICMP},
+		{"icmpv6", iputil.IPProtocolICMPv6},
+		{iputil.IPProtocolICMP, iputil.IPProtocolICMP},
+		{iputil.IPProtocolICMPv6, iputil.IPProtocolICMPv6},
+	} {
 		conf = config.NewC(test.NewLogger())
 		mf = &mockFirewall{}
-		conf.Settings["firewall"] = map[string]any{"outbound": []any{map[string]any{"port": "1", "proto": proto, "host": "a"}}}
-		require.NoError(t, addRulesFromConfig(l, false, conf, mf), proto)
-		assert.Equal(t, addRuleCall{incoming: false, proto: iputil.IPProtocolICMP, startPort: PortAny, endPort: PortAny, groups: nil, host: "a", ip: "", localIp: ""}, mf.lastCall, proto)
+		conf.Settings["firewall"] = map[string]any{"outbound": []any{map[string]any{"port": "1", "proto": tc.given, "host": "a"}}}
+		require.NoError(t, addRulesFromConfig(l, false, conf, mf), tc.given)
+		assert.Equal(t, addRuleCall{incoming: false, proto: tc.want, startPort: PortAny, endPort: PortAny, groups: nil, host: "a", ip: "", localIp: ""}, mf.lastCall, tc.given)
 	}
 
 	// Test protocols that aren't a known name or a number from 1 to 255

firewall/rules_builder.go

@@ -4,40 +4,32 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
-	"hash"
 	"hash/fnv"
-	"io"
 	"log/slog"
 	"math"
 	"net/netip"
 	"slices"
+	"strings"
 
 	"github.com/slackhq/nebula/iputil"
 )
 
 // RulesBuilder collects the inbound and outbound rules, Build turns them into a Table for each direction
 type RulesBuilder struct {
-	in, out tableRules
+	// in and out are each direction's rules in the order they were added
+	in, out []rule
 
 	// defaultLocalCIDRAny makes rules without a local cidr allow any local address, even with unsafe networks
 	defaultLocalCIDRAny bool
 
-	// ruleSHA and ruleFNV hash every rule as it's added, see Rules
-	ruleSHA hash.Hash
-	ruleFNV hash.Hash32
+	// ruleText describes every rule as it is added, one line each. Build hashes it; see Rules.
+	ruleText strings.Builder
 
 	l *slog.Logger
 }
 
-// tableRules are one direction's rules in the order they were added, waiting to be built into a Table
-type tableRules []rule
-
 func NewRulesBuilder(l *slog.Logger) *RulesBuilder {
-	return &RulesBuilder{
-		ruleSHA: sha256.New(),
-		ruleFNV: fnv.New32a(),
-		l:       l,
-	}
+	return &RulesBuilder{l: l}
 }
 
 // AddRule adds a rule for incoming or outgoing traffic, on proto or on every protocol when proto is ProtoAny.
@@ -57,19 +49,19 @@ func (b *RulesBuilder) AddRule(incoming bool, proto uint8, startPort int32, endP
 		endPort = PortAny
 	}
 
-	r, err := parseRule(startPort, endPort, groups, host, cidr, localCidr, caName, caSha)
+	r, err := parseRule(proto, startPort, endPort, groups, host, cidr, localCidr, caName, caSha)
 	if err != nil {
 		return err
 	}
 
 	if incoming {
-		b.in.add(proto, r)
+		b.in = append(b.in, r)
 	} else {
-		b.out.add(proto, r)
+		b.out = append(b.out, r)
 	}
 
 	// The rule hashes are of these lines, changing them changes every hash
-	fmt.Fprintf(io.MultiWriter(b.ruleSHA, b.ruleFNV),
+	fmt.Fprintf(&b.ruleText,
 		"incoming: %v, proto: %v, startPort: %v, endPort: %v, groups: %v, host: %v, ip: %v, localIp: %v, caName: %v, caSha: %s\n",
 		incoming, proto, startPort, endPort, groups, host, cidr, localCidr, caName, caSha,
 	)
@@ -94,15 +86,20 @@ func (b *RulesBuilder) Build(vpnNetworks, unsafeNetworks []netip.Prefix) Rules {
 		defaultLocal = append([]netip.Prefix{}, vpnNetworks...)
 	}
 
+	text := []byte(b.ruleText.String())
+	sha := sha256.Sum256(text)
+	h := fnv.New32a()
+	h.Write(text)
+
 	return Rules{
-		In:      b.in.build(defaultLocal),
-		Out:     b.out.build(defaultLocal),
-		Hash:    hex.EncodeToString(b.ruleSHA.Sum(nil)),
-		HashFNV: b.ruleFNV.Sum32(),
+		In:      buildTable(b.in, defaultLocal),
+		Out:     buildTable(b.out, defaultLocal),
+		Hash:    hex.EncodeToString(sha[:]),
+		HashFNV: h.Sum32(),
 	}
 }
 
-func parseRule(startPort, endPort int32, groups []string, host, cidr, localCidr, caName, caSha string) (rule, error) {
+func parseRule(proto uint8, startPort, endPort int32, groups []string, host, cidr, localCidr, caName, caSha string) (rule, error) {
 	if startPort > endPort {
 		return rule{}, fmt.Errorf("start port was lower than end port")
 	}
@@ -115,6 +112,7 @@ func parseRule(startPort, endPort int32, groups []string, host, cidr, localCidr,
 	}
 
 	r := rule{
+		proto:     proto,
 		startPort: startPort,
 		endPort:   endPort,
 		groups:    groups,
@@ -146,18 +144,12 @@ func parseRule(startPort, endPort int32, groups []string, host, cidr, localCidr,
 	return r, nil
 }
 
-// add keeps r for proto, or for every protocol when proto is ProtoAny.
-func (tr *tableRules) add(proto uint8, r rule) {
-	r.proto = proto
-	*tr = append(*tr, r)
-}
-
-// build turns the rules into a Table. Rules without a local cidr allow defaultLocal,
+// buildTable turns one direction's rules into a Table. Rules without a local cidr allow defaultLocal,
 // or any local address when defaultLocal is nil.
-func (tr tableRules) build(defaultLocal []netip.Prefix) *Table {
-	t := &Table{rules: make([]rule, len(tr))}
+func buildTable(rules []rule, defaultLocal []netip.Prefix) *Table {
+	t := &Table{rules: make([]rule, len(rules))}
 	var hasOwn [256]bool
-	for id, r := range tr {
+	for id, r := range rules {
 		t.rules[id] = r.withDefaultLocal(defaultLocal)
 		if r.proto != ProtoAny {
 			hasOwn[r.proto] = true

outside.go

@@ -361,10 +361,11 @@ func parseV6(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 	fp.Fragment = isFragment
 	fp.FragAny = anyFragment
 	fp.IPHdrLen = offset
+	// fp is reused, so the ports are cleared for the packets that have none: fragments, protocols Nebula
+	// doesn't inspect, and ICMPv6 other than echo. Only the cases below set them.
+	fp.RemotePort, fp.LocalPort = 0, 0
 	if isFragment {
 		// Non-first fragments carry no transport header, so we have no ports to read
-		fp.RemotePort = 0
-		fp.LocalPort = 0
 		return nil
 	}
 
@@ -374,15 +375,12 @@ func parseV6(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 		if dataLen < offset+4 {
 			return ErrIPv6PacketTooShort
 		}
-		fp.LocalPort = 0      //incoming vs outgoing doesn't matter for icmpv6
 		switch data[offset] { //icmp type
 		case iputil.ICMPv6TypeEchoRequest, iputil.ICMPv6TypeEchoReply:
 			if dataLen < offset+6 {
 				return ErrIPv6PacketTooShort
 			}
-			fp.RemotePort = binary.BigEndian.Uint16(data[offset+4 : offset+6]) //identifier
-		default:
-			fp.RemotePort = 0
+			fp.RemotePort = binary.BigEndian.Uint16(data[offset+4 : offset+6]) //identifier, orientation doesn't matter
 		}
 
 	case iputil.HasPorts(proto):
@@ -396,11 +394,6 @@ func parseV6(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 			fp.LocalPort = binary.BigEndian.Uint16(data[offset : offset+2])
 			fp.RemotePort = binary.BigEndian.Uint16(data[offset+2 : offset+4])
 		}
-
-	default:
-		// don't set ports for protocols Nebula doesn't inspect
-		fp.RemotePort = 0
-		fp.LocalPort = 0
 	}
 
 	return nil
@@ -453,16 +446,16 @@ func parseV4(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 		fp.RemoteAddr, _ = netip.AddrFromSlice(data[16:20])
 	}
 
+	// fp is reused, so the ports are cleared for the packets that have none: fragments, and protocols Nebula
+	// doesn't inspect, whose first 4 bytes (GRE, IPIP, AH, etc.) are not ports. Only the cases below set them.
+	fp.RemotePort, fp.LocalPort = 0, 0
 	if fp.Fragment {
-		fp.RemotePort = 0
-		fp.LocalPort = 0
 		return nil
 	}
 
 	switch {
 	case fp.Protocol == iputil.IPProtocolICMP: //note that orientation doesn't matter on ICMP
-		fp.RemotePort = binary.BigEndian.Uint16(data[ihl+4 : ihl+6]) //identifier
-		fp.LocalPort = 0                                             //code would be uint16(data[ihl+1])
+		fp.RemotePort = binary.BigEndian.Uint16(data[ihl+4 : ihl+6]) //identifier, LocalPort stays 0 but could be the code, data[ihl+1]
 
 	case iputil.HasPorts(fp.Protocol):
 		if incoming {
@@ -472,11 +465,6 @@ func parseV4(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 			fp.LocalPort = binary.BigEndian.Uint16(data[ihl : ihl+2])    //src port
 			fp.RemotePort = binary.BigEndian.Uint16(data[ihl+2 : ihl+4]) //dst port
 		}
-
-	default:
-		// don't set ports for protocols Nebula doesn't inspect, the first 4 bytes of GRE, IPIP, AH, etc. are not ports
-		fp.RemotePort = 0
-		fp.LocalPort = 0
 	}
 
 	return nil