From 358cd3fd9299de119f828ef2c1dd5ebd3d160a8c Mon Sep 17 00:00:00 2001
From: David Anderson <danderson@tailscale.com>
Date: Fri, 31 Jul 2020 20:03:00 +0000
Subject: [PATCH] ipn: fix incorrect change tracking for packet filter.

ORder of operations to trigger a problem:
 - Start an already authed tailscaled, verify you can ping stuff.
 - Run `tailscale up`. Notice you can no longer ping stuff.

The problem is that `tailscale up` stops the IPN state machine before
restarting it, which zeros out the packet filter but _not_ the packet
filter hash. Then, upon restarting IPN, the uncleared hash incorrectly
makes the code conclude that the filter doesn't need updating, and so
we stay with a zero filter (reject everything) for ever.

The fix is simply to update the filterHash correctly in all cases,
so that running -> stopped -> running correctly changes the filter
at every transition.

Signed-off-by: David Anderson <danderson@tailscale.com>
---
 ipn/local.go | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/ipn/local.go b/ipn/local.go
index 6c120bfbe..4a529d5ff 100644
--- a/ipn/local.go
+++ b/ipn/local.go
@@ -439,38 +439,45 @@ func (b *LocalBackend) Start(opts Options) error {
 // updateFilter updates the packet filter in wgengine based on the
 // given netMap and user preferences.
 func (b *LocalBackend) updateFilter(netMap *controlclient.NetworkMap, prefs *Prefs) {
-	if netMap == nil {
-		// Not configured yet, block everything
-		b.logf("netmap packet filter: (not ready yet)")
-		b.e.SetFilter(filter.NewAllowNone(b.logf))
-		return
+	// NOTE(danderson): keep change detection as the first thing in
+	// this function. Don't try to optimize by returning early, more
+	// likely than not you'll just end up breaking the change
+	// detection and end up with the wrong filter installed. This is
+	// quite hard to debug, so save yourself the trouble.
+	var (
+		haveNetmap   = netMap != nil
+		addrs        []wgcfg.CIDR
+		packetFilter filter.Matches
+		advRoutes    []wgcfg.CIDR
+		shieldsUp    = prefs == nil || prefs.ShieldsUp // Be conservative when not ready
+	)
+	if haveNetmap {
+		addrs = netMap.Addresses
+		packetFilter = netMap.PacketFilter
 	}
-	packetFilter := netMap.PacketFilter
-
-	var advRoutes []wgcfg.CIDR
 	if prefs != nil {
 		advRoutes = prefs.AdvertiseRoutes
 	}
-	// Be conservative while not ready.
-	shieldsUp := prefs == nil || prefs.ShieldsUp
 
-	changed := deepprint.UpdateHash(&b.filterHash, packetFilter, advRoutes, shieldsUp)
+	changed := deepprint.UpdateHash(&b.filterHash, haveNetmap, addrs, packetFilter, advRoutes, shieldsUp)
 	if !changed {
 		return
 	}
 
 	localNets := wgCIDRsToFilter(netMap.Addresses, advRoutes)
 
-	if shieldsUp {
-		// Shields up, block everything
+	switch {
+	case !haveNetmap:
+		b.logf("netmap packet filter: (not ready yet)")
+		b.e.SetFilter(filter.NewAllowNone(b.logf))
+	case shieldsUp:
 		b.logf("netmap packet filter: (shields up)")
 		var prevFilter *filter.Filter // don't reuse old filter state
 		b.e.SetFilter(filter.New(filter.Matches{}, localNets, prevFilter, b.logf))
-		return
+	default:
+		b.logf("netmap packet filter: %v", packetFilter)
+		b.e.SetFilter(filter.New(packetFilter, localNets, b.e.GetFilter(), b.logf))
 	}
-
-	b.logf("netmap packet filter: %v", packetFilter)
-	b.e.SetFilter(filter.New(packetFilter, localNets, b.e.GetFilter(), b.logf))
 }
 
 // dnsCIDRsEqual determines whether two CIDR lists are equal