tailcfg: ping request notes WIP

The earlier eb06ec172f fixed
the flaky SSH issue (tailscale/corp#1725) by making sure that packets
addressed to Tailscale IPs in hybrid netstack mode weren't delivered
to netstack, but another issue remained:

All traffic handled by netstack was also potentially being handled by
the host networking stack, as the filter hook returned "Accept", which
made it keep processing. This could lead to various random racey chaos
as a function of OS/firewalls/routes/etc.

Instead, once we inject into netstack, stop our caller's packet
processing.

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

cmd/tailscale/cli/cli_test.go

@@ -274,6 +274,71 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			},
 			want: accidentalUpPrefix + " --advertise-routes=11.1.43.0/24,0.0.0.0/0 --advertise-exit-node",
 		},
+		{
+			name:    "advertise_exit_node", // Issue 1859
+			flagSet: f("advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL: ipn.DefaultControlURL,
+					AdvertiseRoutes: []netaddr.IPPrefix{
+						netaddr.MustParseIPPrefix("0.0.0.0/0"),
+						netaddr.MustParseIPPrefix("::/0"),
+					},
+				},
+				// Note: without setting "AdvertiseRoutesSet", as
+				// updateMaskedPrefsFromUpFlag doesn't set that.
+			},
+			want: "",
+		},
+		{
+			name:    "advertise_exit_node_over_existing_routes",
+			flagSet: f("advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("1.2.0.0/16"),
+				},
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL: ipn.DefaultControlURL,
+					AdvertiseRoutes: []netaddr.IPPrefix{
+						netaddr.MustParseIPPrefix("0.0.0.0/0"),
+						netaddr.MustParseIPPrefix("::/0"),
+					},
+				},
+				// Note: without setting "AdvertiseRoutesSet", as
+				// updateMaskedPrefsFromUpFlag doesn't set that.
+			},
+			want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16",
+		},
+		{
+			name:    "advertise_exit_node_over_existing_routes_and_exit_node",
+			flagSet: f("advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("0.0.0.0/0"),
+					netaddr.MustParseIPPrefix("::/0"),
+					netaddr.MustParseIPPrefix("1.2.0.0/16"),
+				},
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL: ipn.DefaultControlURL,
+					AdvertiseRoutes: []netaddr.IPPrefix{
+						netaddr.MustParseIPPrefix("0.0.0.0/0"),
+						netaddr.MustParseIPPrefix("::/0"),
+					},
+				},
+				// Note: without setting "AdvertiseRoutesSet", as
+				// updateMaskedPrefsFromUpFlag doesn't set that.
+			},
+			want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16",
+		},
 		{
 			name:    "exit_node_clearing", // Issue 1777
 			flagSet: f("exit-node"),
@@ -317,7 +382,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 					WantRunning: true,
 				},
 			},
-			want: accidentalUpPrefix + " --accept-routes --exit-node=100.64.5.6 --accept-dns --shields-up --advertise-tags=tag:foo,tag:bar --hostname=myhostname --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice",
+			want: accidentalUpPrefix + " --force-reauth --accept-routes --exit-node=100.64.5.6 --accept-dns --shields-up --advertise-tags=tag:foo,tag:bar --hostname=myhostname --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice",
 		},
 		{
 			name:    "remove_all_implicit_except_hostname",
@@ -426,7 +491,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 }
 
 func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs {
-	upArgs := defaultUpArgsByGOOS(goos)
+	upArgs := upArgsFromOSArgs(goos)
 	prefs, err := prefsFromUpArgs(upArgs, logger.Discard, new(ipnstate.Status), "linux")
 	if err != nil {
 		t.Fatalf("defaultPrefsFromUpArgs: %v", err)
@@ -435,9 +500,9 @@ func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs {
 	return prefs
 }
 
-func defaultUpArgsByGOOS(goos string) (args upArgsT) {
+func upArgsFromOSArgs(goos string, flagArgs ...string) (args upArgsT) {
 	fs := newUpFlagSet(goos, &args)
-	fs.Parse(nil) // populates args
+	fs.Parse(flagArgs) // populates args
 	return
 }
 
@@ -454,7 +519,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 		{
 			name: "default_linux",
 			goos: "linux",
-			args: defaultUpArgsByGOOS("linux"),
+			args: upArgsFromOSArgs("linux"),
 			want: &ipn.Prefs{
 				ControlURL:       ipn.DefaultControlURL,
 				WantRunning:      true,
@@ -467,7 +532,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 		{
 			name: "default_windows",
 			goos: "windows",
-			args: defaultUpArgsByGOOS("windows"),
+			args: upArgsFromOSArgs("windows"),
 			want: &ipn.Prefs{
 				ControlURL:       ipn.DefaultControlURL,
 				WantRunning:      true,
@@ -476,6 +541,21 @@ func TestPrefsFromUpArgs(t *testing.T) {
 				NetfilterMode:    preftype.NetfilterOn,
 			},
 		},
+		{
+			name: "advertise_default_route",
+			args: upArgsFromOSArgs("linux", "--advertise-exit-node"),
+			want: &ipn.Prefs{
+				ControlURL:       ipn.DefaultControlURL,
+				WantRunning:      true,
+				AllowSingleHosts: true,
+				CorpDNS:          true,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("0.0.0.0/0"),
+					netaddr.MustParseIPPrefix("::/0"),
+				},
+				NetfilterMode: preftype.NetfilterOn,
+			},
+		},
 		{
 			name: "error_advertise_route_invalid_ip",
 			args: upArgsT{

cmd/tailscale/cli/up.go

@@ -587,6 +587,16 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			flagExplicitValue[flagName] = ev.Field(i).Interface()
 			continue
 		}
+
+		if prefName == "AdvertiseRoutes" &&
+			(len(curPrefs.AdvertiseRoutes) == 0 ||
+				hasExitNodeRoutes(curPrefs.AdvertiseRoutes) && len(curPrefs.AdvertiseRoutes) == 2) &&
+			hasExitNodeRoutes(mp.Prefs.AdvertiseRoutes) &&
+			len(mp.Prefs.AdvertiseRoutes) == 2 &&
+			flagSet["advertise-exit-node"] {
+			continue
+		}
+
 		// Get explicit value and implicit value
 		ex, im := ev.Field(i), iv.Field(i)
 		switch ex.Kind() {
@@ -624,6 +634,9 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			if prefName == "ExitNodeIP" {
 				missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi)))
 			}
+		case "advertise-routes":
+			routes := withoutExitNodes(exi.([]netaddr.IPPrefix))
+			missing = append(missing, fmtFlagValueArg("advertise-routes", fmtSettingVal(routes)))
 		default:
 			missing = append(missing, fmtFlagValueArg(flagName, fmtSettingVal(exi)))
 		}
@@ -642,6 +655,8 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 	for _, flagName := range flagSetSorted {
 		if ev, ok := flagExplicitValue[flagName]; ok {
 			fmt.Fprintf(&sb, " %s", fmtFlagValueArg(flagName, fmtSettingVal(ev)))
+		} else {
+			fmt.Fprintf(&sb, " --%s", flagName)
 		}
 	}
 	for _, a := range missing {
@@ -698,3 +713,19 @@ func hasExitNodeRoutes(rr []netaddr.IPPrefix) bool {
 	}
 	return v4 && v6
 }
+
+// withoutExitNodes returns rr unchanged if it has only 1 or 0 /0
+// routes. If it has both IPv4 and IPv6 /0 routes, then it returns
+// a copy with all /0 routes removed.
+func withoutExitNodes(rr []netaddr.IPPrefix) []netaddr.IPPrefix {
+	if !hasExitNodeRoutes(rr) {
+		return rr
+	}
+	var out []netaddr.IPPrefix
+	for _, r := range rr {
+		if r.Bits > 0 {
+			out = append(out, r)
+		}
+	}
+	return out
+}

net/tstun/wrap.go

@@ -457,13 +457,22 @@ func (t *Wrapper) filterIn(buf []byte) filter.Response {
 // like wireguard-go/tun.Device.Write.
 func (t *Wrapper) Write(buf []byte, offset int) (int, error) {
 	if !t.disableFilter {
-		res := t.filterIn(buf[offset:])
-		if res == filter.DropSilently {
+		if t.filterIn(buf[offset:]) != filter.Accept {
+			// If we're not accepting the packet, lie to wireguard-go and pretend
+			// that everything is okay with a nil error, so wireguard-go
+			// doesn't log about this Write "failure".
+			//
+			// We return len(buf), but the ill-defined wireguard-go/tun.Device.Write
+			// method doesn't specify how the offset affects the return value.
+			// In fact, the Linux implementation does one of two different things depending
+			// on how the /dev/net/tun was created. But fortunately the wireguard-go
+			// code ignores the int return and only looks at the error:
+			//
+			//     device/receive.go: _, err = device.tun.device.Write(....)
+			//
+			// TODO(bradfitz): fix upstream interface docs, implementation.
 			return len(buf), nil
 		}
-		if res != filter.Accept {
-			return 0, ErrFiltered
-		}
 	}
 
 	t.noteActivity()

net/tstun/wrap_test.go

@@ -329,11 +329,14 @@ func TestFilter(t *testing.T) {
 			var filtered bool
 
 			if tt.dir == in {
+				// Use the side effect of updating the last
+				// activity atomic to determine whether the
+				// data was actually filtered.
+				// If it stays zero, nothing made it through
+				// to the wrapped TUN.
+				atomic.StoreInt64(&tun.lastActivityAtomic, 0)
 				_, err = tun.Write(tt.data, 0)
-				if err == ErrFiltered {
-					filtered = true
-					err = nil
-				}
+				filtered = atomic.LoadInt64(&tun.lastActivityAtomic) == 0
 			} else {
 				chtun.Outbound <- tt.data
 				n, err = tun.Read(buf[:], 0)

tailcfg/tailcfg.go

@@ -884,6 +884,24 @@ type PingRequest struct {
 	// Log is whether to log about this ping in the success case.
 	// For failure cases, the client will log regardless.
 	Log bool `json:",omitempty"`
+
+	Initiator        string // admin@email; "system" (for Tailscale)
+	TestIP           netaddr.IP
+	Types            string // empty means all: TSMP+ICMP+disco
+	StopAfterNDirect int    // 1 means stop on 1st direct ping; 4 means 4 direct pings; 0 means do MaxPings and stop
+	MaxPings         int    // MaxPings total, direct or DERPed
+	PayloadSize      int    // default: 0 extra bytes
+}
+
+type StreamedPingResult struct {
+	IP      netaddr.IP
+	SeqNum  int     // somewhat redundant with TxID but for clarity
+	SentTo  NodeID  // for exit/subnet relays
+	TxID    string  // N hex bytes random
+	Dir     string  // "in"/"out"
+	Type    string  // ICMP, disco, TSMP, ...
+	Via     string  // "direct", "derp-nyc", ...
+	Seconds float64 // for Dir "in" only
 }
 
 type MapResponse struct {

wgengine/netstack/netstack.go

@@ -410,7 +410,14 @@ func (ns *Impl) injectInbound(p *packet.Parsed, t *tstun.Wrapper) filter.Respons
 		Data: vv,
 	})
 	ns.linkEP.InjectInbound(pn, packetBuf)
-	return filter.Accept
+
+	// We've now delivered this to netstack, so we're done.
+	// Instead of returning a filter.Accept here (which would also
+	// potentially deliver it to the host OS), and instead of
+	// filter.Drop (which would log about rejected traffic),
+	// instead return filter.DropSilently which just quietly stops
+	// processing it in the tstun TUN wrapper.
+	return filter.DropSilently
 }
 
 func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) {