From afcf1348124db09e5cf1eedbceea35fbcec681eb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 14 Dec 2020 08:21:41 -0800 Subject: [PATCH] wgengine/filter, tailcfg: support CIDRs+ranges in PacketFilter (mapver 7) Signed-off-by: Brad Fitzpatrick --- control/controlclient/direct.go | 2 +- go.mod | 2 +- go.sum | 4 ++ tailcfg/tailcfg.go | 32 ++++++---- wgengine/filter/filter_test.go | 28 ++++++--- wgengine/filter/tailcfg.go | 100 +++++++++++++++++++++++--------- 6 files changed, 116 insertions(+), 52 deletions(-) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 4d8d6d577..5d33af8fb 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -545,7 +545,7 @@ func (c *Direct) PollNetMap(ctx context.Context, maxPolls int, cb func(*NetworkM } request := tailcfg.MapRequest{ - Version: 6, + Version: 7, KeepAlive: c.keepAlive, NodeKey: tailcfg.NodeKey(persist.PrivateNodeKey.Public()), DiscoKey: c.discoPubKey, diff --git a/go.mod b/go.mod index 1d3deb67c..8ee65a939 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,6 @@ require ( golang.org/x/tools v0.0.0-20201211185031-d93e913c1a58 golang.zx2c4.com/wireguard/windows v0.1.2-0.20201113162609-9b85be97fdf8 honnef.co/go/tools v0.1.0 - inet.af/netaddr v0.0.0-20200810144936-56928fe48a98 + inet.af/netaddr v0.0.0-20201123222344-3c8588dd0e81 rsc.io/goversion v1.2.0 ) diff --git a/go.sum b/go.sum index 47ef6c628..46e8724f4 100644 --- a/go.sum +++ b/go.sum @@ -103,6 +103,8 @@ github.com/tailscale/depaware v0.0.0-20201210233412-71b54857b5d9 h1:IquU2Mhy4Q+x github.com/tailscale/depaware v0.0.0-20201210233412-71b54857b5d9/go.mod h1:jissDaJNHiyV2tFdr3QyNEfsZrax/i2yQiSO+CljThI= github.com/tailscale/depaware v0.0.0-20201214215404-77d1e9757027 h1:lK99QQdH3yBWY6aGilF+IRlQIdmhzLrsEmF6JgN+Ryw= github.com/tailscale/depaware v0.0.0-20201214215404-77d1e9757027/go.mod h1:p9lPsd+cx33L3H9nNoecRRxPssFKUwwI50I3pZ0yT+8= +github.com/tailscale/wireguard-go v0.0.0-20201210001956-32a957fb6709 h1:cxiYxd+Kb+LuXBpv6rp2CpWGhhcVB5b07B6h+kA7LP4= +github.com/tailscale/wireguard-go v0.0.0-20201210001956-32a957fb6709/go.mod h1:9PbAnF5CAklkURoO0uQhm+YUjDmm9T9oCyTGlCHuTPQ= github.com/tcnksm/go-httpstat v0.2.0 h1:rP7T5e5U2HfmOBmZzGgGZjBQ5/GluWUylujl0tJ04I0= github.com/tcnksm/go-httpstat v0.2.0/go.mod h1:s3JVJFtQxtBEBC9dwcdTTXS9xFnM3SXAZwPG41aurT8= github.com/toqueteos/webbrowser v1.2.0 h1:tVP/gpK69Fx+qMJKsLE7TD8LuGWPnEV71wBN9rrstGQ= @@ -219,5 +221,7 @@ honnef.co/go/tools v0.1.0 h1:AWNL1W1i7f0wNZ8VwOKNJ0sliKvOF/adn0EHenfUh+c= honnef.co/go/tools v0.1.0/go.mod h1:XtegFAyX/PfluP4921rXU5IkjkqBCDnUq4W8VCIoKvM= inet.af/netaddr v0.0.0-20200810144936-56928fe48a98 h1:bWyWDZP0l6VnQ1TDKf6yNwuiEDV6Q3q1Mv34m+lzT1I= inet.af/netaddr v0.0.0-20200810144936-56928fe48a98/go.mod h1:qqYzz/2whtrbWJvt+DNWQyvekNN4ePQZcg2xc2/Yjww= +inet.af/netaddr v0.0.0-20201123222344-3c8588dd0e81 h1:cNn1RTwcpyBtspGkbBh6EnK/njkZvl+AutIKUgw/LRA= +inet.af/netaddr v0.0.0-20201123222344-3c8588dd0e81/go.mod h1:qqYzz/2whtrbWJvt+DNWQyvekNN4ePQZcg2xc2/Yjww= rsc.io/goversion v1.2.0 h1:SPn+NLTiAG7w30IRK/DKp1BjvpWabYgxlLp/+kx5J8w= rsc.io/goversion v1.2.0/go.mod h1:Eih9y/uIBS3ulggl7KNJ09xGSLcuNaLgmvvqa07sgfo= diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 191ec0615..bbe56fe00 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -477,6 +477,7 @@ type MapRequest struct { // 4: opt-in keep-alives via KeepAlive field, opt-in compression via Compress // 5: 2020-10-19, implies IncludeIPv6, delta Peers/UserProfiles, supports MagicDNS // 6: 2020-12-07: means MapResponse.PacketFilter nil means unchanged + // 7: 2020-12-15: FilterRule.SrcIPs accepts CIDRs+ranges, doesn't warn about 0.0.0.0/:: Version int Compress string // "zstd" or "" (no compression) KeepAlive bool // whether server should send keep-alives back to us @@ -526,11 +527,11 @@ type PortRange struct { var PortRangeAny = PortRange{0, 65535} -// NetPortRange represents a single subnet:portrange. +// NetPortRange represents a range of ports that's allowed for one or more IPs. type NetPortRange struct { _ structs.Incomparable - IP string // "*" means all - Bits *int // backward compatibility: if missing, means "all" bits + IP string // IP, CIDR, Range, or "*" (same formats as FilterRule.SrcIPs) + Bits *int // deprecated; the old way to turn IP into a CIDR Ports PortRange } @@ -541,18 +542,25 @@ type NetPortRange struct { // allowed if a source IP is mathces of those CIDRs. type FilterRule struct { // SrcIPs are the source IPs/networks to match. - // The special value "*" means to match all. + // + // It may take the following forms: + // * an IP address (IPv4 or IPv6) + // * the string "*" to match everything (both IPv4 & IPv6) + // * a CIDR (e.g. "192.168.0.0/16") + // * a range of two IPs, inclusive, separated by hyphen ("2eff::1-2eff::0800") SrcIPs []string - // SrcBits values correspond to the SrcIPs above. + // SrcBits is deprecated; it's the old way to specify a CIDR + // prior to MapRequest.Version 7. Its values correspond to the + // SrcIPs above. // - // If present at the same index, it changes the SrcIP above to - // be a network with /n CIDR bits. If the slice is nil or - // insufficiently long, the default value (for an IPv4 - // address) for a position is 32, as if the SrcIPs above were - // a /32 mask. For a "*" SrcIPs value, the corresponding - // SrcBits value is ignored. - // TODO: for IPv6, clarify default bits length. + // If an entry of SrcBits is present for the same index as a + // SrcIPs entry, it changes the SrcIP above to be a network + // with /n CIDR bits. If the slice is nil or insufficiently + // long, the default value (for an IPv4 address) for a + // position is 32, as if the SrcIPs above were a /32 mask. For + // a "*" SrcIPs value, the corresponding SrcBits value is + // ignored. SrcBits []int // DstPorts are the port ranges to allow once a source IP diff --git a/wgengine/filter/filter_test.go b/wgengine/filter/filter_test.go index 75403e461..dffcf5f0b 100644 --- a/wgengine/filter/filter_test.go +++ b/wgengine/filter/filter_test.go @@ -194,7 +194,7 @@ func TestNoAllocs(t *testing.T) { } } -func TestParseIP(t *testing.T) { +func TestParseIPSet(t *testing.T) { tests := []struct { host string bits int @@ -203,23 +203,33 @@ func TestParseIP(t *testing.T) { }{ {"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 host "8.8.8.8"`}, - {"8.8.8.8", -1, nil, `invalid CIDR size -1 for host "8.8.8.8"`}, - {"2601:1234::", 129, nil, `invalid CIDR size 129 for host "2601:1234::"`}, - {"0.0.0.0", 24, nil, `ports="0.0.0.0": to allow all IP addresses, use *:port, not 0.0.0.0:port`}, - {"::", 64, nil, `ports="::": to allow all IP addresses, use *:port, not [::]:port`}, + {"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"), ""}, } for _, tt := range tests { - got, err := parseIP(tt.host, tt.bits) + var bits *int + if tt.bits != -1 { + bits = &tt.bits + } + got, err := parseIPSet(tt.host, bits) if err != nil { if err.Error() == tt.wantErr { continue } - t.Errorf("parseIP(%q, %v) error: %v; want error %q", tt.host, tt.bits, err, tt.wantErr) + t.Errorf("parseIPSet(%q, %v) error: %v; want error %q", tt.host, tt.bits, err, tt.wantErr) } if diff := cmp.Diff(got, tt.want, cmp.Comparer(func(a, b netaddr.IP) bool { return a == b })); diff != "" { - t.Errorf("parseIP(%q, %v) = %s; want %s", tt.host, tt.bits, got, tt.want) + t.Errorf("parseIPSet(%q, %v) = %s; want %s", tt.host, tt.bits, got, tt.want) continue } } diff --git a/wgengine/filter/tailcfg.go b/wgengine/filter/tailcfg.go index db3b28469..807a1d407 100644 --- a/wgengine/filter/tailcfg.go +++ b/wgengine/filter/tailcfg.go @@ -6,6 +6,7 @@ package filter import ( "fmt" + "strings" "inet.af/netaddr" "tailscale.com/tailcfg" @@ -22,11 +23,11 @@ func MatchesFromFilterRules(pf []tailcfg.FilterRule) ([]Match, error) { m := Match{} for i, s := range r.SrcIPs { - bits := 32 + var bits *int if len(r.SrcBits) > i { - bits = r.SrcBits[i] + bits = &r.SrcBits[i] } - nets, err := parseIP(s, bits) + nets, err := parseIPSet(s, bits) if err != nil && erracc == nil { erracc = err continue @@ -35,11 +36,7 @@ func MatchesFromFilterRules(pf []tailcfg.FilterRule) ([]Match, error) { } for _, d := range r.DstPorts { - bits := 32 - if d.Bits != nil { - bits = *d.Bits - } - nets, err := parseIP(d.IP, bits) + nets, err := parseIPSet(d.IP, d.Bits) if err != nil && erracc == nil { erracc = err continue @@ -65,35 +62,80 @@ var ( zeroIP6 = netaddr.IPFrom16([16]byte{}) ) -func parseIP(host string, defaultBits int) ([]netaddr.IPPrefix, error) { - if host == "*" { - // User explicitly requested wildcard dst ip. +// parseIPSet parses arg as one: +// +// * an IP address (IPv4 or IPv6) +// * the string "*" to match everything (both IPv4 & IPv6) +// * 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) ([]netaddr.IPPrefix, error) { + if arg == "*" { + // User explicitly requested wildcard. return []netaddr.IPPrefix{ {IP: zeroIP4, Bits: 0}, {IP: zeroIP6, Bits: 0}, }, nil } - - ip, err := netaddr.ParseIP(host) - if err != nil { - return nil, fmt.Errorf("ports=%#v: invalid IP address", host) + if strings.Contains(arg, "/") { + pfx, err := netaddr.ParseIPPrefix(arg) + if err != nil { + return nil, err + } + if pfx != pfx.Masked() { + return nil, fmt.Errorf("%v contains non-network bits set", pfx) + } + return []netaddr.IPPrefix{pfx}, nil } - if ip == zeroIP4 { - // For clarity, reject 0.0.0.0 as an input - return nil, fmt.Errorf("ports=%#v: to allow all IP addresses, use *:port, not 0.0.0.0:port", host) + if strings.Count(arg, "-") == 1 { + i := strings.Index(arg, "-") + ip1s, ip2s := arg[:i], arg[i+1:] + ip1, err := netaddr.ParseIP(ip1s) + if err != nil { + return nil, err + } + ip2, err := netaddr.ParseIP(ip2s) + if err != nil { + return nil, err + } + r := netaddr.IPRange{From: ip1, To: ip2} + if !r.Valid() { + return nil, fmt.Errorf("invalid IP range %q", arg) + } + return r.Prefixes(), nil } - if ip == zeroIP6 { - // For clarity, reject :: as an input - return nil, fmt.Errorf("ports=%#v: to allow all IP addresses, use *:port, not [::]:port", host) + ip, err := netaddr.ParseIP(arg) + if err != nil { + return nil, fmt.Errorf("invalid IP address %q", arg) } - if defaultBits < 0 || (ip.Is4() && defaultBits > 32) || (ip.Is6() && defaultBits > 128) { - return nil, fmt.Errorf("invalid CIDR size %d for host %q", defaultBits, host) + var bits8 uint8 + if ip.Is4() { + bits8 = 32 + if bits != nil { + if *bits < 0 || *bits > 32 { + return nil, fmt.Errorf("invalid CIDR size %d for IP %q", *bits, arg) + } + bits8 = uint8(*bits) + } + } else if ip.Is6() { + bits8 = 128 + if bits != nil { + if *bits < 0 || *bits > 128 { + return nil, fmt.Errorf("invalid CIDR size %d for IP %q", *bits, arg) + } + bits8 = uint8(*bits) + } + } + if bits8 == 0 { + return nil, fmt.Errorf("unknown IP type %q", ip) } - return []netaddr.IPPrefix{ - { - IP: ip, - Bits: uint8(defaultBits), - }, - }, nil + return []netaddr.IPPrefix{{IP: ip, Bits: bits8}}, nil }