From ed8ae57fee00f5b34064edf24ee258e1dd94eb90 Mon Sep 17 00:00:00 2001 From: JackDoan Date: Wed, 30 Sep 2026 15:06:35 -0500 Subject: [PATCH] checkpt --- firewall/config.go | 9 ++---- firewall/config_test.go | 18 ++++++++---- firewall/rules_builder.go | 58 +++++++++++++++++---------------------- outside.go | 28 ++++++------------- 4 files changed, 49 insertions(+), 64 deletions(-) diff --git a/firewall/config.go b/firewall/config.go index aabb2aca..9e5b76e9 100644 --- a/firewall/config.go +++ b/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 != "" { diff --git a/firewall/config_test.go b/firewall/config_test.go index be5f98ba..f22e7226 100644 --- a/firewall/config_test.go +++ b/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 diff --git a/firewall/rules_builder.go b/firewall/rules_builder.go index 4f93a683..1bc624c2 100644 --- a/firewall/rules_builder.go +++ b/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 diff --git a/outside.go b/outside.go index 6cb6676c..f61fb52d 100644 --- a/outside.go +++ b/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