wgengine/router: windows: set SkipAsSource on IPv6 LL addresses

raggi · raggi · commit 76256d22d865 · 2022-06-22T15:26:40.000-07:00
Link-local addresses on the Tailscale interface are not routable. Ideally they would be removed, however, a concern exists that the operating system will attempt to re-add them which would lead to thrashing. Setting SkipAsSource attempts to avoid production of packets using the address as a source in any default behaviors. Before, in powershell: `ping (hostname)` would ping the link-local address of the Tailscale interface, and fail. After: `ping (hostname)` now pings the link-local address on the next highest priority metric local interface. Fixes tailscale#4647 Signed-off-by: James Tucker <james@tailscale.com>
diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go
@@ -574,26 +574,8 @@ func deltaNets(a, b []*net.IPNet) (add, del []*net.IPNet) {
 	return
 }
 
-func excludeIPv6LinkLocal(in []*net.IPNet) (out []*net.IPNet) {
-	out = in[:0]
-	for _, n := range in {
-		if len(n.IP) == 16 && n.IP.IsLinkLocalUnicast() {
-			// Windows creates a fixed link-local address for wintun,
-			// which doesn't seem to route correctly. Unfortunately, LLMNR returns this
-			// address for lookups by the hostname, and Windows prefers using it.
-			// This means that local traffic addressed to the machine's hostname breaks.
-			//
-			// While we otherwise preserve link-local addresses, we delete
-			// this one to force lookups to use a working address.
-			//
-			// See: https://github.com/tailscale/tailscale/issues/4647
-			if ip, ok := netaddr.FromStdIP(n.IP); !ok || wintunLinkLocal != ip {
-				continue // filter this IPNet
-			}
-		}
-		out = append(out, n)
-	}
-	return out
+func isIPv6LinkLocal(in *net.IPNet) bool {
+	return len(in.IP) == 16 && in.IP.IsLinkLocalUnicast()
 }
 
 // ipAdapterUnicastAddressToIPNet converts windows.IpAdapterUnicastAddress to net.IPNet.
@@ -622,14 +604,27 @@ func unicastIPNets(ifc *winipcfg.IPAdapterAddresses) []*net.IPNet {
 // doing the minimum number of AddAddresses & DeleteAddress calls.
 // This avoids the full FlushAddresses.
 //
-// Any IPv6 link-local addresses are not deleted.
+// Any IPv6 link-local addresses are not deleted out of caution as some
+// configurations may repeatedly re-add them. Link-local addresses are adjusted
+// to set SkipAsSource. SkipAsSource prevents the addresses from being addded to
+// DNS locally or remotely and from being picked as a source address for
+// outgoing packets with unspecified sources. See #4647 and
+// https://web.archive.org/web/20200912120956/https://devblogs.microsoft.com/scripting/use-powershell-to-change-ip-behavior-with-skipassource/
 func syncAddresses(ifc *winipcfg.IPAdapterAddresses, want []*net.IPNet) error {
 	var erracc error
 
 	got := unicastIPNets(ifc)
 	add, del := deltaNets(got, want)
-	del = excludeIPv6LinkLocal(del)
+
+	ll := make([]*net.IPNet, 0)
 	for _, a := range del {
+		// do not delete link-local addresses, and collect them for later
+		// applying SkipAsSource.
+		if isIPv6LinkLocal(a) {
+			ll = append(ll, a)
+			continue
+		}
+
 		err := ifc.LUID.DeleteIPAddress(*a)
 		if err != nil {
 			erracc = fmt.Errorf("deleting IP %q: %w", *a, err)
@@ -643,6 +638,20 @@ func syncAddresses(ifc *winipcfg.IPAdapterAddresses, want []*net.IPNet) error {
 		}
 	}
 
+	for _, a := range ll {
+		mib, err := ifc.LUID.IPAddress(a.IP)
+		if err != nil {
+			erracc = fmt.Errorf("setting skip-as-source on IP %q: unable to retrieve MIB: %w", *a, err)
+			continue
+		}
+		if !mib.SkipAsSource {
+			mib.SkipAsSource = true
+			if err := mib.Set(); err != nil {
+				erracc = fmt.Errorf("setting skip-as-source on IP %q: unable to set MIB: %w", *a, err)
+			}
+		}
+	}
+
 	return erracc
 }
 
diff --git a/wgengine/router/ifconfig_windows_test.go b/wgengine/router/ifconfig_windows_test.go
@@ -163,11 +163,6 @@ func TestDeltaNets(t *testing.T) {
 			b:       nets("100.84.36.11/32[4]"),
 			wantDel: nets("fe80::99d0:ec2d:b2e7:536b/64"),
 		},
-		{
-			a:       excludeIPv6LinkLocal(nets("100.84.36.11/32", "fe80::99d0:ec2d:b2e7:536b/64")),
-			b:       nets("100.84.36.11/32"),
-			wantDel: nets("fe80::99d0:ec2d:b2e7:536b/64"),
-		},
 		{
 			a: []*net.IPNet{
 				{

Original file line number	Diff line number	Diff line change
`@@ -163,11 +163,6 @@ func TestDeltaNets(t *testing.T) {`
`163`	`163`	`b: nets("100.84.36.11/32[4]"),`
`164`	`164`	`wantDel: nets("fe80::99d0:ec2d:b2e7:536b/64"),`
`165`	`165`	`},`
`166`		`- {`
`167`		`- a: excludeIPv6LinkLocal(nets("100.84.36.11/32", "fe80::99d0:ec2d:b2e7:536b/64")),`
`168`		`- b: nets("100.84.36.11/32"),`
`169`		`- wantDel: nets("fe80::99d0:ec2d:b2e7:536b/64"),`
`170`		`- },`
`171`	`166`	`{`
`172`	`167`	`a: []*net.IPNet{`
`173`	`168`	`{`