From e99e48cca73e4b5820107c5b9d765041c490e8ed Mon Sep 17 00:00:00 2001 From: JackDoan Date: Wed, 30 Sep 2026 14:57:19 -0500 Subject: [PATCH] checkpt --- firewall/config.go | 25 +++++----------- firewall/config_test.go | 6 ++-- firewall/packet.go | 60 ++++++++++++++++++++++++++------------- firewall/rules_builder.go | 34 +++++++++++----------- firewall/table.go | 13 +-------- iputil/packet.go | 10 +++++++ outside.go | 12 ++++---- 7 files changed, 83 insertions(+), 77 deletions(-) diff --git a/firewall/config.go b/firewall/config.go index ef320f58..aabb2aca 100644 --- a/firewall/config.go +++ b/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 != "" { diff --git a/firewall/config_test.go b/firewall/config_test.go index a39ef2d9..be5f98ba 100644 --- a/firewall/config_test.go +++ b/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 diff --git a/firewall/packet.go b/firewall/packet.go index c8a10186..7b9f64b0 100644 --- a/firewall/packet.go +++ b/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 diff --git a/firewall/rules_builder.go b/firewall/rules_builder.go index d9c14d4e..4f93a683 100644 --- a/firewall/rules_builder.go +++ b/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 diff --git a/firewall/table.go b/firewall/table.go index 702b096e..f3e1e0f8 100644 --- a/firewall/table.go +++ b/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 { diff --git a/iputil/packet.go b/iputil/packet.go index 0b7cad3a..85c25ca5 100644 --- a/iputil/packet.go +++ b/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 diff --git a/outside.go b/outside.go index 3ae326fd..6cb6676c 100644 --- a/outside.go +++ b/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