feature/conn25: send ICMP errors back to sources

if we can't find a mapping for the magic IP they're sending traffic to.

Fixes tailscale/corp#34257

Signed-off-by: Fran Bull <fran@tailscale.com>

feature/conn25/conn25.go

@@ -207,11 +207,11 @@ func (e *extension) installHooks(dph *datapathHandler) error {
 
 	// Intercept packets from the tun device and from WireGuard
 	// to perform DNAT and SNAT.
-	tun.PreFilterPacketOutboundToWireGuardAppConnectorIntercept = func(p *packet.Parsed, _ *tstun.Wrapper) filter.Response {
+	tun.PreFilterPacketOutboundToWireGuardAppConnectorIntercept = func(p *packet.Parsed, tun *tstun.Wrapper) filter.Response {
 		if !e.conn25.isConfigured() {
 			return filter.Accept
 		}
-		return dph.HandlePacketFromTunDevice(p)
+		return dph.HandlePacketFromTunDevice(p, tun)
 	}
 	tun.PostFilterPacketInboundFromWireGuardAppConnector = func(p *packet.Parsed, tun *tstun.Wrapper) filter.Response {
 		if !e.conn25.isConfigured() {

feature/conn25/datapath.go

@@ -204,7 +204,7 @@ func (dh *datapathHandler) HandlePacketFromWireGuard(p *packet.Parsed, tun *tstu
 // that the packet should pass through subsequent stages of the datapath pipeline.
 // Returning [filter.Drop] signals the packet should be dropped. This method handles all
 // packets coming from the tun device, on both connectors, and clients of connectors.
-func (dh *datapathHandler) HandlePacketFromTunDevice(p *packet.Parsed) filter.Response {
+func (dh *datapathHandler) HandlePacketFromTunDevice(p *packet.Parsed, tun *tstun.Wrapper) filter.Response {
 	if !isSupportedProtocol(p.IPProto) {
 		return filter.Accept
 	}
@@ -233,7 +233,9 @@ func (dh *datapathHandler) HandlePacketFromTunDevice(p *packet.Parsed) filter.Re
 	transitIP, err := dh.conn25.ClientTransitIPForMagicIP(magicIP)
 	if err != nil {
 		if errors.Is(err, ErrUnmappedMagicIP) {
-			// TODO(tailscale/corp#34257): This path should deliver an ICMP error to the client.
+			// Couldn't find a mapping. Tell the local application the host is
+			// unreachable so it can recover quickly instead of blackholing.
+			dh.sendICMPHostUnreachable(p, tun)
 			return filter.Drop
 		}
 		dh.debugLogf("error mapping magic IP, passing packet unmodified: %v", err)
@@ -273,6 +275,22 @@ func (dh *datapathHandler) HandlePacketFromTunDevice(p *packet.Parsed) filter.Re
 	return filter.Accept
 }
 
+// sendICMPHostUnreachable injects an ICMP "host unreachable" error (or its IPv6
+// equivalent, "address unreachable") back to the local host in response to the
+// tun-device packet p. The error is delivered inbound so the local
+// application that sent p sees it, giving it the chance to recover.
+func (dh *datapathHandler) sendICMPHostUnreachable(p *packet.Parsed, tun *tstun.Wrapper) {
+	// The error appears to come from the unreachable Magic IP, addressed back to
+	// the sender, and embeds data from the invoking packet.
+	errPkt := packet.GenerateICMPHostUnreachable(p.Dst.Addr(), p.Src.Addr(), p)
+	if errPkt == nil {
+		return
+	}
+	if err := tun.InjectInboundCopy(errPkt); err != nil {
+		dh.debugLogf("error injecting ICMP host unreachable packet: %v", err)
+	}
+}
+
 func (dh *datapathHandler) dnatAction(to netip.Addr) PacketAction {
 	return PacketAction(func(p *packet.Parsed) { checksum.UpdateDstAddr(p, to) })
 }

feature/conn25/datapath_test.go

@@ -8,7 +8,9 @@ import (
 	"errors"
 	"net/netip"
 	"testing"
+	"time"
 
+	"github.com/tailscale/wireguard-go/tun/tuntest"
 	"go4.org/netipx"
 	"tailscale.com/net/packet"
 	"tailscale.com/net/tstun"
@@ -123,12 +125,14 @@ func TestHandlePacketFromTunDevice(t *testing.T) {
 				return netip.Addr{}, nil
 			}
 			dph := newDatapathHandler(mock, t.Logf)
+			tun := newFakeTUN(t)
+			defer tun.Close()
 
 			tt.p.IPProto = ipproto.UDP
 			tt.p.IPVersion = 4
 			tt.p.StuffForTesting(40)
 
-			if want, got := tt.expectedFilterResponse, dph.HandlePacketFromTunDevice(tt.p); want != got {
+			if want, got := tt.expectedFilterResponse, dph.HandlePacketFromTunDevice(tt.p, tun); want != got {
 				t.Errorf("unexpected filter response: want %v, got %v", want, got)
 			}
 			if want, got := tt.expectedSrc, tt.p.Src; want != got {
@@ -141,6 +145,95 @@ func TestHandlePacketFromTunDevice(t *testing.T) {
 	}
 }
 
+// TestUnmappedMagicIPICMPUnreachable verifies that a packet to a Magic IP with
+// no active Transit IP mapping is dropped and an ICMP host-unreachable error is
+// injected back toward the local host, sourced from the Magic IP and addressed
+// to the original sender.
+func TestUnmappedMagicIPICMPUnreachable(t *testing.T) {
+	const clientPort, serverPort = 1234, 80
+
+	tests := []struct {
+		name        string
+		clientSrcIP netip.Addr
+		magicIP     netip.Addr
+		wantProto   ipproto.Proto
+	}{
+		{
+			name:        "ipv4",
+			clientSrcIP: netip.MustParseAddr("100.70.0.1"),
+			magicIP:     netip.MustParseAddr("10.64.0.2"),
+			wantProto:   ipproto.ICMPv4,
+		},
+		{
+			name:        "ipv6",
+			clientSrcIP: netip.MustParseAddr("fd7a:115c:a1e0::1"),
+			magicIP:     netip.MustParseAddr("fd7a:115c:a1e0::2"),
+			wantProto:   ipproto.ICMPv6,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mock := &testConn25{}
+			mock.clientTransitIPForMagicIPFn = func(netip.Addr) (netip.Addr, error) {
+				return netip.Addr{}, ErrUnmappedMagicIP
+			}
+			dph := newDatapathHandler(mock, t.Logf)
+			chtun, tun := newChannelTUN(t)
+			defer tun.Close()
+
+			var raw []byte
+			if tt.magicIP.Is6() {
+				raw = packet.Generate(packet.UDP6Header{
+					IP6Header: packet.IP6Header{Src: tt.clientSrcIP, Dst: tt.magicIP},
+					SrcPort:   clientPort,
+					DstPort:   serverPort,
+				}, []byte("hello"))
+			} else {
+				raw = packet.Generate(packet.UDP4Header{
+					IP4Header: packet.IP4Header{Src: tt.clientSrcIP, Dst: tt.magicIP},
+					SrcPort:   clientPort,
+					DstPort:   serverPort,
+				}, []byte("x"))
+			}
+			var p packet.Parsed
+			p.Decode(raw)
+
+			// HandlePacketFromTunDevice blocks until the injected packet is
+			// read, so drain the channel TUN concurrently.
+			gotInboundPacketChan := make(chan []byte, 1)
+			go func() { gotInboundPacketChan <- <-chtun.Inbound }()
+
+			if got, want := dph.HandlePacketFromTunDevice(&p, tun), filter.Drop; got != want {
+				t.Fatalf("unexpected filter response: got %v, want %v", got, want)
+			}
+
+			var injected packet.Parsed
+			select {
+			case b := <-gotInboundPacketChan:
+				injected.Decode(b)
+			case <-time.After(1 * time.Second):
+				t.Fatal("timed out waiting for injected ICMP packet")
+			}
+
+			if !injected.IsError() {
+				t.Errorf("injected packet is not an ICMP error")
+			}
+			if got := injected.IPProto; got != tt.wantProto {
+				t.Errorf("injected packet proto: got %v, want %v", got, tt.wantProto)
+			}
+			// The error should appear to come from the unreachable Magic IP,
+			// addressed back to the original sender.
+			if got, want := injected.Src.Addr(), tt.magicIP; got != want {
+				t.Errorf("injected packet src: got %v, want %v", got, want)
+			}
+			if got, want := injected.Dst.Addr(), tt.clientSrcIP; got != want {
+				t.Errorf("injected packet dst: got %v, want %v", got, want)
+			}
+		})
+	}
+}
+
 func newFakeTUN(t *testing.T) *tstun.Wrapper {
 	t.Helper()
 
@@ -174,6 +267,40 @@ func newFakeTUN(t *testing.T) *tstun.Wrapper {
 	return tun
 }
 
+// newChannelTUN is like newFakeTUN, but backed by a channel-based TUN device
+// whose Inbound queue captures packets injected toward the local host (e.g. via
+// InjectInboundCopy), so tests can observe them.
+func newChannelTUN(t *testing.T) (*tuntest.ChannelTUN, *tstun.Wrapper) {
+	t.Helper()
+
+	chtun := tuntest.NewChannelTUN()
+	reg := new(usermetric.Registry)
+	bus := eventbustest.NewBus(t)
+	tun := tstun.Wrap(t.Logf, chtun.TUN(), reg, bus)
+
+	protos := views.SliceOf([]ipproto.Proto{
+		ipproto.TCP,
+		ipproto.UDP,
+		ipproto.ICMPv4,
+		ipproto.ICMPv6,
+	})
+	allIPs := netip.MustParsePrefix("0.0.0.0/0")
+	matches := []filter.Match{
+		{
+			IPProto: protos,
+			Srcs:    []netip.Prefix{allIPs},
+			Dsts:    []filtertype.NetPortRange{{Net: allIPs, Ports: filtertype.AllPorts}},
+		},
+	}
+	var sb netipx.IPSetBuilder
+	sb.AddPrefix(allIPs)
+	ipSet, _ := sb.IPSet()
+	tun.SetFilter(filter.New(matches, nil, ipSet, ipSet, nil, t.Logf))
+
+	tun.Start()
+	return chtun, tun
+}
+
 func TestHandlePacketFromWireGuard(t *testing.T) {
 	clientSrcIP := netip.MustParseAddr("100.70.0.1")
 	unknownSrcIP := netip.MustParseAddr("100.99.99.99")
@@ -325,6 +452,8 @@ func TestClientFlowCache(t *testing.T) {
 		return transitIP, nil
 	}
 	dph := newDatapathHandler(mock, t.Logf)
+	tun := newFakeTUN(t)
+	defer tun.Close()
 
 	outgoing := packet.Parsed{
 		IPProto:   ipproto.UDP,
@@ -335,7 +464,7 @@ func TestClientFlowCache(t *testing.T) {
 	outgoing.StuffForTesting(40)
 
 	o1 := outgoing
-	if dph.HandlePacketFromTunDevice(&o1) != filter.Accept {
+	if dph.HandlePacketFromTunDevice(&o1, tun) != filter.Accept {
 		t.Errorf("first call to HandlePacketFromTunDevice was not accepted")
 	}
 	if want, got := netip.AddrPortFrom(transitIP, serverPort), o1.Dst; want != got {
@@ -343,7 +472,7 @@ func TestClientFlowCache(t *testing.T) {
 	}
 	// The second call should use the cache.
 	o2 := outgoing
-	if dph.HandlePacketFromTunDevice(&o2) != filter.Accept {
+	if dph.HandlePacketFromTunDevice(&o2, tun) != filter.Accept {
 		t.Errorf("second call to HandlePacketFromTunDevice was not accepted")
 	}
 	if want, got := netip.AddrPortFrom(transitIP, serverPort), o2.Dst; want != got {
@@ -360,9 +489,6 @@ func TestClientFlowCache(t *testing.T) {
 	}
 	incoming.StuffForTesting(40)
 
-	tun := newFakeTUN(t)
-	defer tun.Close()
-
 	if dph.HandlePacketFromWireGuard(incoming, tun) != filter.Accept {
 		t.Errorf("call to HandlePacketFromWireGuard was not accepted")
 	}
@@ -428,7 +554,7 @@ func TestConnectorFlowCache(t *testing.T) {
 	}
 	incoming.StuffForTesting(40)
 
-	if dph.HandlePacketFromTunDevice(incoming) != filter.Accept {
+	if dph.HandlePacketFromTunDevice(incoming, tun) != filter.Accept {
 		t.Errorf("call to HandlePacketFromTunDevice was not accepted")
 	}
 	if want, got := netip.AddrPortFrom(transitIP, serverPort), incoming.Src; want != got {