wgengine/filter, tailcfg: support CIDRs+ranges in PacketFilter (mapver 7)

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

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,

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
 )

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=

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
+	IP    string // IP, CIDR, Range, or "*" (same formats as FilterRule.SrcIPs)
-	Bits  *int   // backward compatibility: if missing, means "all" bits
+	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
+	// If an entry of SrcBits is present for the same index as a
-	// be a network with /n CIDR bits. If the slice is nil or
+	// SrcIPs entry, it changes the SrcIP above to be a network
-	// insufficiently long, the default value (for an IPv4
+	// with /n CIDR bits. If the slice is nil or insufficiently
-	// address) for a position is 32, as if the SrcIPs above were
+	// long, the default value (for an IPv4 address) for a
-	// a /32 mask. For a "*" SrcIPs value, the corresponding
+	// position is 32, as if the SrcIPs above were a /32 mask. For
-	// SrcBits value is ignored.
+	// a "*" SrcIPs value, the corresponding SrcBits value is
-	// TODO: for IPv6, clarify default bits length.
+	// ignored.
 	SrcBits []int
 
 	// DstPorts are the port ranges to allow once a source IP

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", 33, nil, `invalid CIDR size 33 for IP "8.8.8.8"`},
-		{"8.8.8.8", -1, nil, `invalid CIDR size -1 for host "8.8.8.8"`},
+		{"8.8.8.8", -1, pfx("8.8.8.8/32"), ""},
-		{"2601:1234::", 129, nil, `invalid CIDR size 129 for host "2601:1234::"`},
+		{"8.8.8.8", 32, pfx("8.8.8.8/32"), ""},
-		{"0.0.0.0", 24, nil, `ports="0.0.0.0": to allow all IP addresses, use *:port, not 0.0.0.0:port`},
+		{"8.8.8.8/24", -1, nil, "8.8.8.8/24 contains non-network bits set"},
-		{"::", 64, nil, `ports="::": to allow all IP addresses, use *:port, not [::]:port`},
+		{"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
 		}
 	}

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
+			nets, err := parseIPSet(d.IP, d.Bits)
-			if d.Bits != nil {
-				bits = *d.Bits
-			}
-			nets, err := parseIP(d.IP, 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) {
+// parseIPSet parses arg as one:
-	if host == "*" {
+//
-		// User explicitly requested wildcard dst ip.
+//     * 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
 	}
-
+	if strings.Contains(arg, "/") {
-	ip, err := netaddr.ParseIP(host)
+		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 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
+	}
+	ip, err := netaddr.ParseIP(arg)
 	if err != nil {
-		return nil, fmt.Errorf("ports=%#v: invalid IP address", host)
+		return nil, fmt.Errorf("invalid IP address %q", arg)
-	}
-	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 ip == zeroIP6 {
-		// For clarity, reject :: as an input
-		return nil, fmt.Errorf("ports=%#v: to allow all IP addresses, use *:port, not [::]:port", host)
 	}
 
-	if defaultBits < 0 || (ip.Is4() && defaultBits > 32) || (ip.Is6() && defaultBits > 128) {
+	var bits8 uint8
-		return nil, fmt.Errorf("invalid CIDR size %d for host %q", defaultBits, host)
+	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)
+		}
 	}
-	return []netaddr.IPPrefix{
+	if bits8 == 0 {
-		{
+		return nil, fmt.Errorf("unknown IP type %q", ip)
-			IP:   ip,
+	}
-			Bits: uint8(defaultBits),
+	return []netaddr.IPPrefix{{IP: ip, Bits: bits8}}, nil
-		},
-	}, nil
 }