checkpt

firewall/config.go

@@ -75,23 +75,8 @@ func addRulesFromConfig(l *slog.Logger, inbound bool, c *config.C, ra ruleAdder)
 			sPort = r.Port
 		}
 
-		var proto uint8
-		switch r.Proto {
-		case "any":
-			proto = ProtoAny
-		case "tcp":
-			proto = iputil.IPProtocolTCP
-		case "udp":
-			proto = iputil.IPProtocolUDP
-		case "udplite":
-			proto = iputil.IPProtocolUDPLite
-		case "dccp":
-			proto = iputil.IPProtocolDCCP
-		case "sctp":
-			proto = iputil.IPProtocolSCTP
-		case "icmp":
-			proto = iputil.IPProtocolICMP
-		default:
+		proto, ok := protoByName(r.Proto)
+		if !ok {
 			// Any other protocol by number. 0 is reserved for `any`.
 			n, perr := strconv.ParseUint(r.Proto, 10, 8)
 			if perr != nil || n == 0 {
@@ -99,9 +84,13 @@ 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 || proto == iputil.IPProtocolICMPv6 {
+		if proto == iputil.IPProtocolICMP {
 			startPort = PortAny
 			endPort = PortAny
 			if sPort != "" {

firewall/config_test.go

@@ -170,13 +170,13 @@ 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 number still ignore ports
-	for _, proto := range []uint8{iputil.IPProtocolICMP, iputil.IPProtocolICMPv6} {
+	// 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} {
 		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: proto, startPort: PortAny, endPort: PortAny, groups: nil, host: "a", ip: "", localIp: ""}, mf.lastCall, proto)
+		assert.Equal(t, addRuleCall{incoming: false, proto: iputil.IPProtocolICMP, startPort: PortAny, endPort: PortAny, groups: nil, host: "a", ip: "", localIp: ""}, mf.lastCall, proto)
 	}
 
 	// Test protocols that aren't a known name or a number from 1 to 255

firewall/packet.go

@@ -40,35 +40,55 @@ func (fp *Packet) Copy() *Packet {
 }
 
 func (fp Packet) MarshalJSON() ([]byte, error) {
-	var proto string
-	switch fp.Protocol {
-	case iputil.IPProtocolTCP:
-		proto = "tcp"
-	case iputil.IPProtocolICMP:
-		proto = "icmp"
-	case iputil.IPProtocolICMPv6:
-		proto = "icmpv6"
-	case iputil.IPProtocolUDP:
-		proto = "udp"
-	case iputil.IPProtocolUDPLite:
-		proto = "udplite"
-	case iputil.IPProtocolDCCP:
-		proto = "dccp"
-	case iputil.IPProtocolSCTP:
-		proto = "sctp"
-	default:
-		proto = fmt.Sprintf("unknown %v", fp.Protocol)
-	}
 	return json.Marshal(m{
 		"LocalAddr":  fp.LocalAddr.String(),
 		"RemoteAddr": fp.RemoteAddr.String(),
 		"LocalPort":  fp.LocalPort,
 		"RemotePort": fp.RemotePort,
-		"Protocol":   proto,
+		"Protocol":   protoName(fp.Protocol),
 		"Fragment":   fp.Fragment,
 	})
 }
 
+// protoNames are the protocol names that rules accept and packets report, and their numbers. Any other protocol
+// is given by number.
+var protoNames = []struct {
+	name  string
+	proto uint8
+}{
+	{"any", ProtoAny},
+	{"tcp", iputil.IPProtocolTCP},
+	{"udp", iputil.IPProtocolUDP},
+	{"udplite", iputil.IPProtocolUDPLite},
+	{"dccp", iputil.IPProtocolDCCP},
+	{"sctp", iputil.IPProtocolSCTP},
+	{"icmp", iputil.IPProtocolICMP},
+	{"icmpv6", iputil.IPProtocolICMPv6},
+}
+
+// protoByName returns the number of a protocol named in protoNames.
+func protoByName(name string) (uint8, bool) {
+	for _, p := range protoNames {
+		if p.name == name {
+			return p.proto, true
+		}
+	}
+	return 0, false
+}
+
+// protoName returns the name in protoNames of a packet's protocol, or "unknown" and its number. `any` is only
+// a rule's protocol, so a packet's protocol 0 is unknown.
+func protoName(proto uint8) string {
+	if proto != ProtoAny {
+		for _, p := range protoNames {
+			if p.proto == proto {
+				return p.name
+			}
+		}
+	}
+	return fmt.Sprintf("unknown %v", proto)
+}
+
 // ParsedPacket is a Packet plus the parse byproducts the RX path reuses
 type ParsedPacket struct {
 	Packet

firewall/rules_builder.go

@@ -43,7 +43,12 @@ func NewRulesBuilder(l *slog.Logger) *RulesBuilder {
 // AddRule adds a rule for incoming or outgoing traffic, on proto or on every protocol when proto is ProtoAny.
 // The rule is parsed now, so a bad rule is reported when it's added rather than when the Tables are built.
 func (b *RulesBuilder) AddRule(incoming bool, proto uint8, startPort int32, endPort int32, groups []string, host string, cidr, localCidr, caName string, caSha string) error {
-	if proto == iputil.IPProtocolICMP || proto == iputil.IPProtocolICMPv6 {
+	// ICMPv6 rules are ICMP rules, and a packet of either protocol is checked against both
+	if proto == iputil.IPProtocolICMPv6 {
+		proto = iputil.IPProtocolICMP
+	}
+
+	if proto == iputil.IPProtocolICMP {
 		//ICMP traffic doesn't have ports, so we always coerce to "any", even if a value is provided
 		if startPort != PortAny {
 			b.l.Warn("ignoring port specification for ICMP firewall rule", "startPort", startPort)
@@ -141,11 +146,8 @@ 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. ICMPv6 rules are kept as ICMP rules.
+// add keeps r for proto, or for every protocol when proto is ProtoAny.
 func (tr *tableRules) add(proto uint8, r rule) {
-	if proto == iputil.IPProtocolICMPv6 {
-		proto = iputil.IPProtocolICMP
-	}
 	r.proto = proto
 	*tr = append(*tr, r)
 }
@@ -162,27 +164,23 @@ func (tr tableRules) build(defaultLocal []netip.Prefix) *Table {
 		}
 	}
 
-	// Every protocol starts out sharing an index of the proto `any` rules, including protos[ProtoAny]. There is
-	// one for protocols with ports and one for protocols without.
+	// A protocol with rules of its own gets an index of those rules plus the proto `any` rules, so a packet
+	// checks a single index. The other protocols share an index of the proto `any` rules, one for protocols with
+	// ports and one for protocols without.
 	anyPorts := newProtoIndex(true, t.rules, ProtoAny)
 	anyNoPorts := newProtoIndex(false, t.rules, ProtoAny)
 	for proto := range t.protos {
-		if hasPorts(uint8(proto)) {
+		switch {
+		case hasOwn[proto]:
+			t.protos[proto] = newProtoIndex(iputil.HasPorts(uint8(proto)), t.rules, uint8(proto))
+		case iputil.HasPorts(uint8(proto)):
 			t.protos[proto] = anyPorts
-		} else {
+		default:
 			t.protos[proto] = anyNoPorts
 		}
 	}
 
-	// A protocol with rules of its own gets an index of those rules plus the proto `any` rules, so a packet
-	// checks a single index.
-	for proto, own := range hasOwn {
-		if own {
-			t.protos[proto] = newProtoIndex(hasPorts(uint8(proto)), t.rules, uint8(proto))
-		}
-	}
-
-	// ICMP and ICMPv6 share one set of rules; add keeps them together under ICMP.
+	// ICMPv6 rules are ICMP rules, see AddRule, so ICMPv6 packets are checked against the ICMP index
 	t.protos[iputil.IPProtocolICMPv6] = t.protos[iputil.IPProtocolICMP]
 
 	return t

firewall/table.go

@@ -4,7 +4,6 @@ import (
 	"net/netip"
 
 	"github.com/slackhq/nebula/cert"
-	"github.com/slackhq/nebula/iputil"
 )
 
 // Rules are the Tables built for each direction, with hashes of the rules they were built from
@@ -35,7 +34,7 @@ type Table struct {
 
 // protoIndex holds the rules that can apply to one protocol, in a set for each kind of packet.
 type protoIndex struct {
-	// hasPorts reports whether the protocol has ports; see the hasPorts function.
+	// hasPorts reports whether the protocol has ports; see iputil.HasPorts.
 	hasPorts bool
 	// byPort has a set for every port number of a protocol with ports. Port 0 is covered only by port `any`
 	// rules, since a range starts at 1.
@@ -46,16 +45,6 @@ type protoIndex struct {
 	fragment ruleSet
 }
 
-// hasPorts reports whether Nebula parses port numbers from packets of proto. parseV4 and parseV6 in outside.go
-// must agree. A rule with a port or a port range only applies to these protocols.
-func hasPorts(proto uint8) bool {
-	switch proto {
-	case iputil.IPProtocolTCP, iputil.IPProtocolUDP, iputil.IPProtocolUDPLite, iputil.IPProtocolDCCP, iputil.IPProtocolSCTP:
-		return true
-	}
-	return false
-}
-
 // whichMatch returns the set of rules whose proto and port clauses allow p.
 func (pi *protoIndex) whichMatch(p *Packet, incoming bool) ruleSet {
 	switch {

iputil/packet.go

@@ -39,6 +39,16 @@ const (
 	ICMPv6TypeEchoReply   = 129
 )
 
+// HasPorts reports whether packets of proto carry port numbers that the packet parsers read: TCP, UDP, UDPLite,
+// DCCP, and SCTP. Firewall rules with a port or a port range only apply to these protocols.
+func HasPorts(proto uint8) bool {
+	switch proto {
+	case IPProtocolTCP, IPProtocolUDP, IPProtocolUDPLite, IPProtocolDCCP, IPProtocolSCTP:
+		return true
+	}
+	return false
+}
+
 func CreateRejectPacket(packet []byte, out []byte) []byte {
 	if len(packet) < 1 {
 		return nil

outside.go

@@ -368,8 +368,8 @@ func parseV6(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 		return nil
 	}
 
-	switch proto {
-	case iputil.IPProtocolICMPv6:
+	switch {
+	case proto == iputil.IPProtocolICMPv6:
 		// An ICMPv6 message is at least type, code and checksum, 4 bytes. Only echo carries more than we read.
 		if dataLen < offset+4 {
 			return ErrIPv6PacketTooShort
@@ -385,7 +385,7 @@ func parseV6(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 			fp.RemotePort = 0
 		}
 
-	case iputil.IPProtocolTCP, iputil.IPProtocolUDP, iputil.IPProtocolDCCP, iputil.IPProtocolSCTP, iputil.IPProtocolUDPLite:
+	case iputil.HasPorts(proto):
 		if dataLen < offset+4 {
 			return ErrIPv6PacketTooShort
 		}
@@ -459,12 +459,12 @@ func parseV4(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 		return nil
 	}
 
-	switch fp.Protocol {
-	case iputil.IPProtocolICMP: //note that orientation doesn't matter on ICMP
+	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])
 
-	case iputil.IPProtocolTCP, iputil.IPProtocolUDP, iputil.IPProtocolDCCP, iputil.IPProtocolSCTP, iputil.IPProtocolUDPLite:
+	case iputil.HasPorts(fp.Protocol):
 		if incoming {
 			fp.RemotePort = binary.BigEndian.Uint16(data[ihl : ihl+2])  //src port
 			fp.LocalPort = binary.BigEndian.Uint16(data[ihl+2 : ihl+4]) //dst port