From 21460a5b14e9fb883fedd6071ff53729ed68370c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 18 Jun 2024 14:37:30 -0700 Subject: [PATCH] tailcfg, wgengine/filter: remove most FilterRule.SrcBits code The control plane hasn't sent it to clients in ages. Updates tailscale/corp#20965 Change-Id: I1d71a4b6dd3f75010a05c544ee39827837c30772 Signed-off-by: Brad Fitzpatrick --- tailcfg/tailcfg.go | 16 +++++++++------- util/deephash/deephash_test.go | 7 +++---- wgengine/filter/filter_test.go | 33 +++++++++++---------------------- wgengine/filter/tailcfg.go | 32 ++++++++++++-------------------- 4 files changed, 35 insertions(+), 53 deletions(-) diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 4460eb374..a7eb68d37 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -1333,7 +1333,7 @@ var PortRangeAny = PortRange{0, 65535} type NetPortRange struct { _ structs.Incomparable IP string // IP, CIDR, Range, or "*" (same formats as FilterRule.SrcIPs) - Bits *int // deprecated; the old way to turn IP into a CIDR + Bits *int // deprecated; the 2020 way to turn IP into a CIDR. See FilterRule.SrcBits. Ports PortRange } @@ -1470,7 +1470,7 @@ func (c PeerCapMap) HasCapability(cap PeerCapability) bool { // FilterRule represents one rule in a packet filter. // // A rule is logically a set of source CIDRs to match (described by -// SrcIPs and SrcBits), and a set of destination targets that are then +// SrcIPs), and a set of destination targets that are then // allowed if a source IP is matches of those CIDRs. type FilterRule struct { // SrcIPs are the source IPs/networks to match. @@ -1482,7 +1482,7 @@ type FilterRule struct { // * a range of two IPs, inclusive, separated by hyphen ("2eff::1-2eff::0800") SrcIPs []string - // SrcBits is deprecated; it's the old way to specify a CIDR + // SrcBits is deprecated; it was the old way to specify a CIDR // prior to CapabilityVersion 7. Its values correspond to the // SrcIPs above. // @@ -1493,10 +1493,14 @@ type FilterRule struct { // position is 32, as if the SrcIPs above were a /32 mask. For // a "*" SrcIPs value, the corresponding SrcBits value is // ignored. + // + // This is still present in this file because the Tailscale control plane + // code still uses this type, for 118 clients that are still connected as of + // 2024-06-18, 3.5 years after the last release that used this type. SrcBits []int `json:",omitempty"` // DstPorts are the port ranges to allow once a source IP - // matches (is in the CIDR described by SrcIPs & SrcBits). + // matches (is in the CIDR described by SrcIPs). // // CapGrant and DstPorts are mutually exclusive: at most one can be non-nil. DstPorts []NetPortRange `json:",omitempty"` @@ -1527,11 +1531,9 @@ type FilterRule struct { var FilterAllowAll = []FilterRule{ { - SrcIPs: []string{"*"}, - SrcBits: nil, + SrcIPs: []string{"*"}, DstPorts: []NetPortRange{{ IP: "*", - Bits: nil, Ports: PortRange{0, 65535}, }}, }, diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go index 7908f9a60..1c8d9d604 100644 --- a/util/deephash/deephash_test.go +++ b/util/deephash/deephash_test.go @@ -483,8 +483,8 @@ func TestGetTypeHasher(t *testing.T) { { name: "packet_filter", val: filterRules, - out: "\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x03\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00*\v\x00\x00\x00\x00\x00\x00\x0010.1.3.4/32\v\x00\x00\x00\x00\x00\x00\x0010.0.0.0/24\x01\x03\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\n\x00\x00\x00\x00\x00\x00\x001.2.3.4/32\x01 \x00\x00\x00\x00\x00\x00\x00\x01\x00\x02\x00\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04!\x01\x01\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00foo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00", - out32: "\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x03\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00*\v\x00\x00\x00\x00\x00\x00\x0010.1.3.4/32\v\x00\x00\x00\x00\x00\x00\x0010.0.0.0/24\x01\x03\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\n\x00\x00\x00\x00\x00\x00\x001.2.3.4/32\x01 \x00\x00\x00\x01\x00\x02\x00\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04!\x01\x01\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00foo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00", + out: "\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x03\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00*\v\x00\x00\x00\x00\x00\x00\x0010.1.3.4/32\v\x00\x00\x00\x00\x00\x00\x0010.0.0.0/24\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\n\x00\x00\x00\x00\x00\x00\x001.2.3.4/32\x01 \x00\x00\x00\x00\x00\x00\x00\x01\x00\x02\x00\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04!\x01\x01\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00foo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00", + out32: "\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x03\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00*\v\x00\x00\x00\x00\x00\x00\x0010.1.3.4/32\v\x00\x00\x00\x00\x00\x00\x0010.0.0.0/24\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\n\x00\x00\x00\x00\x00\x00\x001.2.3.4/32\x01 \x00\x00\x00\x00\x00\x00\x00\x01\x00\x02\x00\x01\x04\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04!\x01\x01\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00foo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\v\x00\x00\x00\x00\x00\x00\x00foooooooooo\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\f\x00\x00\x00\x00\x00\x00\x00baaaaaarrrrr\x00\x01\x00\x02\x00\x00\x00", }, { name: "netip.Addr", @@ -771,8 +771,7 @@ func BenchmarkHash(b *testing.B) { // packet filters as sent to clients. var filterRules = []tailcfg.FilterRule{ { - SrcIPs: []string{"*", "10.1.3.4/32", "10.0.0.0/24"}, - SrcBits: []int{1, 2, 3}, + SrcIPs: []string{"*", "10.1.3.4/32", "10.0.0.0/24"}, DstPorts: []tailcfg.NetPortRange{{ IP: "1.2.3.4/32", Bits: ptr.To(32), diff --git a/wgengine/filter/filter_test.go b/wgengine/filter/filter_test.go index 1fa4173f4..082476371 100644 --- a/wgengine/filter/filter_test.go +++ b/wgengine/filter/filter_test.go @@ -251,41 +251,30 @@ func TestNoAllocs(t *testing.T) { func TestParseIPSet(t *testing.T) { tests := []struct { host string - bits int want []netip.Prefix wantErr string }{ - {"8.8.8.8", 24, pfx("8.8.8.8/24"), ""}, - {"2601:1234::", 64, pfx("2601:1234::/64"), ""}, - {"8.8.8.8", 33, nil, `invalid CIDR size 33 for IP "8.8.8.8"`}, - {"8.8.8.8", -1, pfx("8.8.8.8/32"), ""}, - {"8.8.8.8", 32, pfx("8.8.8.8/32"), ""}, - {"8.8.8.8/24", -1, nil, "8.8.8.8/24 contains non-network bits set"}, - {"8.8.8.0/24", 18, pfx("8.8.8.0/24"), ""}, // the 18 is ignored - {"1.0.0.0-1.255.255.255", 5, pfx("1.0.0.0/8"), ""}, - {"1.0.0.0-2.1.2.3", 5, pfx("1.0.0.0/8", "2.0.0.0/16", "2.1.0.0/23", "2.1.2.0/30"), ""}, - {"1.0.0.2-1.0.0.1", -1, nil, "invalid IP range \"1.0.0.2-1.0.0.1\""}, - {"2601:1234::", 129, nil, `invalid CIDR size 129 for IP "2601:1234::"`}, - {"0.0.0.0", 24, pfx("0.0.0.0/24"), ""}, - {"::", 64, pfx("::/64"), ""}, - {"*", 24, pfx("0.0.0.0/0", "::/0"), ""}, + {"8.8.8.8", pfx("8.8.8.8/32"), ""}, + {"1::2", pfx("1::2/128"), ""}, + {"8.8.8.0/24", pfx("8.8.8.0/24"), ""}, + {"8.8.8.8/24", nil, "8.8.8.8/24 contains non-network bits set"}, + {"1.0.0.0-1.255.255.255", pfx("1.0.0.0/8"), ""}, + {"1.0.0.0-2.1.2.3", pfx("1.0.0.0/8", "2.0.0.0/16", "2.1.0.0/23", "2.1.2.0/30"), ""}, + {"1.0.0.2-1.0.0.1", nil, "invalid IP range \"1.0.0.2-1.0.0.1\""}, + {"*", pfx("0.0.0.0/0", "::/0"), ""}, } for _, tt := range tests { - var bits *int - if tt.bits != -1 { - bits = &tt.bits - } - got, err := parseIPSet(tt.host, bits) + got, err := parseIPSet(tt.host) if err != nil { if err.Error() == tt.wantErr { continue } - t.Errorf("parseIPSet(%q, %v) error: %v; want error %q", tt.host, tt.bits, err, tt.wantErr) + t.Errorf("parseIPSet(%q) error: %v; want error %q", tt.host, err, tt.wantErr) } compareIP := cmp.Comparer(func(a, b netip.Addr) bool { return a == b }) compareIPPrefix := cmp.Comparer(func(a, b netip.Prefix) bool { return a == b }) if diff := cmp.Diff(got, tt.want, compareIP, compareIPPrefix); diff != "" { - t.Errorf("parseIPSet(%q, %v) = %s; want %s", tt.host, tt.bits, got, tt.want) + t.Errorf("parseIPSet(%q) = %s; want %s", tt.host, got, tt.want) continue } } diff --git a/wgengine/filter/tailcfg.go b/wgengine/filter/tailcfg.go index be9c6f13b..ab77ea315 100644 --- a/wgengine/filter/tailcfg.go +++ b/wgengine/filter/tailcfg.go @@ -33,6 +33,9 @@ func MatchesFromFilterRules(pf []tailcfg.FilterRule) ([]Match, error) { var erracc error for _, r := range pf { + if len(r.SrcBits) > 0 { + return nil, fmt.Errorf("unexpected SrcBits; control plane should not send this to this client version") + } // Profiling determined that this function was spending a lot // of time in runtime.growslice. As such, we attempt to // pre-allocate some slices. Multipliers were chosen arbitrarily. @@ -54,12 +57,8 @@ func MatchesFromFilterRules(pf []tailcfg.FilterRule) ([]Match, error) { m.IPProto = views.SliceOf(filtered) } - for i, s := range r.SrcIPs { - var bits *int - if len(r.SrcBits) > i { - bits = &r.SrcBits[i] - } - nets, err := parseIPSet(s, bits) + for _, s := range r.SrcIPs { + nets, err := parseIPSet(s) if err != nil && erracc == nil { erracc = err continue @@ -69,7 +68,10 @@ func MatchesFromFilterRules(pf []tailcfg.FilterRule) ([]Match, error) { m.SrcsContains = ipset.NewContainsIPFunc(views.SliceOf(m.Srcs)) for _, d := range r.DstPorts { - nets, err := parseIPSet(d.IP, d.Bits) + if d.Bits != nil { + return nil, fmt.Errorf("unexpected DstBits; control plane should not send this to this client version") + } + nets, err := parseIPSet(d.IP) if err != nil && erracc == nil { erracc = err continue @@ -119,14 +121,11 @@ var ( // - a CIDR (e.g. "192.168.0.0/16") // - a range of two IPs, inclusive, separated by hyphen ("2eff::1-2eff::0800") // -// bits, if non-nil, is the legacy SrcBits CIDR length to make a IP -// address (without a slash) treated as a CIDR of *bits length. -// // TODO(bradfitz): make this return an IPSet and plumb that all // around, and ultimately use a new version of IPSet.ContainsFunc like // Contains16Func that works in [16]byte address, so we we can match // at runtime without allocating? -func parseIPSet(arg string, bits *int) ([]netip.Prefix, error) { +func parseIPSet(arg string) ([]netip.Prefix, error) { if arg == "*" { // User explicitly requested wildcard. return []netip.Prefix{ @@ -155,7 +154,7 @@ func parseIPSet(arg string, bits *int) ([]netip.Prefix, error) { return nil, err } r := netipx.IPRangeFrom(ip1, ip2) - if !r.Valid() { + if !r.IsValid() { return nil, fmt.Errorf("invalid IP range %q", arg) } return r.Prefixes(), nil @@ -164,12 +163,5 @@ func parseIPSet(arg string, bits *int) ([]netip.Prefix, error) { if err != nil { return nil, fmt.Errorf("invalid IP address %q", arg) } - bits8 := uint8(ip.BitLen()) - if bits != nil { - if *bits < 0 || *bits > int(bits8) { - return nil, fmt.Errorf("invalid CIDR size %d for IP %q", *bits, arg) - } - bits8 = uint8(*bits) - } - return []netip.Prefix{netip.PrefixFrom(ip, int(bits8))}, nil + return []netip.Prefix{netip.PrefixFrom(ip, ip.BitLen())}, nil }