net/dns, net/dns/resolver, wgengine: refactor DNS request path (#4364)

* net/dns, net/dns/resolver, wgengine: refactor DNS request path

Previously, method calls into the DNS manager/resolver types handled DNS
requests rather than DNS packets. This is fine for UDP as one packet
corresponds to one request or response, however will not suit an
implementation that supports DNS over TCP.

To support PRs implementing this in the future, wgengine delegates
all handling/construction of packets to the magic DNS endpoint, to
the DNS types themselves. Handling IP packets at this level enables
future support for both UDP and TCP.

Signed-off-by: Tom DNetto <tom@tailscale.com>

net/dns/manager.go

@@ -14,6 +14,7 @@ import (
 	"tailscale.com/net/dns/resolver"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/types/dnstype"
+	"tailscale.com/types/ipproto"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/dnsname"
 	"tailscale.com/wgengine/monitor"
@@ -204,12 +205,12 @@ func toIPPorts(ips []netaddr.IP) (ret []netaddr.IPPort) {
 	return ret
 }
 
-func (m *Manager) EnqueueRequest(bs []byte, from netaddr.IPPort) error {
+func (m *Manager) EnqueuePacket(bs []byte, proto ipproto.Proto, from, to netaddr.IPPort) error {
-	return m.resolver.EnqueueRequest(bs, from)
+	return m.resolver.EnqueuePacket(bs, proto, from, to)
 }
 
-func (m *Manager) NextResponse() ([]byte, netaddr.IPPort, error) {
+func (m *Manager) NextPacket() ([]byte, error) {
-	return m.resolver.NextResponse()
+	return m.resolver.NextPacket()
 }
 
 func (m *Manager) Down() error {

net/dns/resolver/tsdns.go

@@ -25,9 +25,12 @@ import (
 	dns "golang.org/x/net/dns/dnsmessage"
 	"inet.af/netaddr"
 	"tailscale.com/net/dns/resolvconffile"
+	tspacket "tailscale.com/net/packet"
 	"tailscale.com/net/tsaddr"
 	"tailscale.com/net/tsdial"
+	"tailscale.com/net/tstun"
 	"tailscale.com/types/dnstype"
+	"tailscale.com/types/ipproto"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
 	"tailscale.com/util/dnsname"
@@ -36,6 +39,13 @@ import (
 
 const dnsSymbolicFQDN = "magicdns.localhost-tailscale-daemon."
 
+var (
+	magicDNSIP   = tsaddr.TailscaleServiceIP()
+	magicDNSIPv6 = tsaddr.TailscaleServiceIPv6()
+)
+
+const magicDNSPort = 53
+
 // maxResponseBytes is the maximum size of a response from a Resolver. The
 // actual buffer size will be one larger than this so that we can detect
 // truncation in a platform-agnostic way.
@@ -282,10 +292,20 @@ func (r *Resolver) Close() {
 	r.forwarder.Close()
 }
 
-// EnqueueRequest places the given DNS request in the resolver's queue.
+// EnqueuePacket handles a packet to the magicDNS endpoint.
 // It takes ownership of the payload and does not block.
 // If the queue is full, the request will be dropped and an error will be returned.
-func (r *Resolver) EnqueueRequest(bs []byte, from netaddr.IPPort) error {
+func (r *Resolver) EnqueuePacket(bs []byte, proto ipproto.Proto, from, to netaddr.IPPort) error {
+	if to.Port() != magicDNSPort || proto != ipproto.UDP {
+		return nil
+	}
+
+	return r.enqueueRequest(bs, proto, from, to)
+}
+
+// enqueueRequest places the given DNS request in the resolver's queue.
+// If the queue is full, the request will be dropped and an error will be returned.
+func (r *Resolver) enqueueRequest(bs []byte, proto ipproto.Proto, from, to netaddr.IPPort) error {
 	metricDNSQueryLocal.Add(1)
 	select {
 	case <-r.closed:
@@ -302,9 +322,56 @@ func (r *Resolver) EnqueueRequest(bs []byte, from netaddr.IPPort) error {
 	return nil
 }
 
-// NextResponse returns a DNS response to a previously enqueued request.
+// NextPacket returns the next packet to service traffic for magicDNS. The returned
+// packet is prefixed with unused space consistent with the semantics of injection
+// into tstun.Wrapper.
+// It blocks until a response is available and gives up ownership of the response payload.
+func (r *Resolver) NextPacket() (ipPacket []byte, err error) {
+	bs, to, err := r.nextResponse()
+	if err != nil {
+		return nil, err
+	}
+
+	// Unused space is needed further down the stack. To avoid extra
+	// allocations/copying later on, we allocate such space here.
+	const offset = tstun.PacketStartOffset
+
+	var buf []byte
+	switch {
+	case to.IP().Is4():
+		h := tspacket.UDP4Header{
+			IP4Header: tspacket.IP4Header{
+				Src: magicDNSIP,
+				Dst: to.IP(),
+			},
+			SrcPort: magicDNSPort,
+			DstPort: to.Port(),
+		}
+		hlen := h.Len()
+		buf = make([]byte, offset+hlen+len(bs))
+		copy(buf[offset+hlen:], bs)
+		h.Marshal(buf[offset:])
+	case to.IP().Is6():
+		h := tspacket.UDP6Header{
+			IP6Header: tspacket.IP6Header{
+				Src: magicDNSIPv6,
+				Dst: to.IP(),
+			},
+			SrcPort: magicDNSPort,
+			DstPort: to.Port(),
+		}
+		hlen := h.Len()
+		buf = make([]byte, offset+hlen+len(bs))
+		copy(buf[offset+hlen:], bs)
+		h.Marshal(buf[offset:])
+	}
+
+	return buf, nil
+}
+
+// nextResponse returns a DNS response to a previously enqueued request.
 // It blocks until a response is available and gives up ownership of the response payload.
-func (r *Resolver) NextResponse() (packet []byte, to netaddr.IPPort, err error) {
+func (r *Resolver) nextResponse() (packet []byte, to netaddr.IPPort, err error) {
 	select {
 	case <-r.closed:
 		return nil, netaddr.IPPort{}, ErrClosed

net/dns/resolver/tsdns_test.go

@@ -27,6 +27,7 @@ import (
 	"tailscale.com/net/tsdial"
 	"tailscale.com/tstest"
 	"tailscale.com/types/dnstype"
+	"tailscale.com/types/ipproto"
 	"tailscale.com/util/dnsname"
 	"tailscale.com/wgengine/monitor"
 )
@@ -37,6 +38,8 @@ var (
 
 	testipv4Arpa = dnsname.FQDN("4.3.2.1.in-addr.arpa.")
 	testipv6Arpa = dnsname.FQDN("f.0.e.0.d.0.c.0.b.0.a.0.9.0.8.0.7.0.6.0.5.0.4.0.3.0.2.0.1.0.0.0.ip6.arpa.")
+
+	magicDNSv4Port = netaddr.MustParseIPPort("100.100.100.100:53")
 )
 
 var dnsCfg = Config{
@@ -231,10 +234,10 @@ func unpackResponse(payload []byte) (dnsResponse, error) {
 }
 
 func syncRespond(r *Resolver, query []byte) ([]byte, error) {
-	if err := r.EnqueueRequest(query, netaddr.IPPort{}); err != nil {
+	if err := r.enqueueRequest(query, ipproto.UDP, netaddr.IPPort{}, magicDNSv4Port); err != nil {
-		return nil, fmt.Errorf("EnqueueRequest: %w", err)
+		return nil, fmt.Errorf("enqueueRequest: %w", err)
 	}
-	payload, _, err := r.NextResponse()
+	payload, _, err := r.nextResponse()
 	return payload, err
 }
 
@@ -727,14 +730,14 @@ func TestDelegateCollision(t *testing.T) {
 	// packets will have the same dns txid.
 	for _, p := range packets {
 		payload := dnspacket(p.qname, p.qtype, noEdns)
-		err := r.EnqueueRequest(payload, p.addr)
+		err := r.enqueueRequest(payload, ipproto.UDP, p.addr, magicDNSv4Port)
 		if err != nil {
 			t.Error(err)
 		}
 	}
 
 	// Despite the txid collision, the answer(s) should still match the query.
-	resp, addr, err := r.NextResponse()
+	resp, addr, err := r.nextResponse()
 	if err != nil {
 		t.Error(err)
 	}

wgengine/userspace.go

@@ -487,23 +487,22 @@ func (e *userspaceEngine) handleLocalPackets(p *packet.Parsed, t *tstun.Wrapper)
 
 // handleDNS is an outbound pre-filter resolving Tailscale domains.
 func (e *userspaceEngine) handleDNS(p *packet.Parsed, t *tstun.Wrapper) filter.Response {
-	if p.Dst.Port() == magicDNSPort && p.IPProto == ipproto.UDP {
+	switch p.Dst.IP() {
-		switch p.Dst.IP() {
+	case magicDNSIP, magicDNSIPv6:
-		case magicDNSIP, magicDNSIPv6:
+		err := e.dns.EnqueuePacket(append([]byte(nil), p.Payload()...), p.IPProto, p.Src, p.Dst)
-			err := e.dns.EnqueueRequest(append([]byte(nil), p.Payload()...), p.Src)
+		if err != nil {
-			if err != nil {
+			e.logf("dns: enqueue: %v", err)
-				e.logf("dns: enqueue: %v", err)
-			}
-			return filter.Drop
 		}
+		return filter.Drop
+	default:
+		return filter.Accept
 	}
-	return filter.Accept
 }
 
-// pollResolver reads responses from the DNS resolver and injects them inbound.
+// pollResolver reads packets from the DNS resolver and injects them inbound.
 func (e *userspaceEngine) pollResolver() {
 	for {
-		bs, to, err := e.dns.NextResponse()
+		bs, err := e.dns.NextPacket()
 		if err == resolver.ErrClosed {
 			return
 		}
@@ -512,39 +511,9 @@ func (e *userspaceEngine) pollResolver() {
 			continue
 		}
 
-		var buf []byte
+		// The leading empty space required by the semantics of
-		const offset = tstun.PacketStartOffset
+		// InjectInboundDirect is allocated in NextPacket().
-		switch {
+		e.tundev.InjectInboundDirect(bs, tstun.PacketStartOffset)
-		case to.IP().Is4():
-			h := packet.UDP4Header{
-				IP4Header: packet.IP4Header{
-					Src: magicDNSIP,
-					Dst: to.IP(),
-				},
-				SrcPort: magicDNSPort,
-				DstPort: to.Port(),
-			}
-			hlen := h.Len()
-			// TODO(dmytro): avoid this allocation without importing tstun quirks into dns.
-			buf = make([]byte, offset+hlen+len(bs))
-			copy(buf[offset+hlen:], bs)
-			h.Marshal(buf[offset:])
-		case to.IP().Is6():
-			h := packet.UDP6Header{
-				IP6Header: packet.IP6Header{
-					Src: magicDNSIPv6,
-					Dst: to.IP(),
-				},
-				SrcPort: magicDNSPort,
-				DstPort: to.Port(),
-			}
-			hlen := h.Len()
-			// TODO(dmytro): avoid this allocation without importing tstun quirks into dns.
-			buf = make([]byte, offset+hlen+len(bs))
-			copy(buf[offset+hlen:], bs)
-			h.Marshal(buf[offset:])
-		}
-		e.tundev.InjectInboundDirect(buf, offset)
 	}
 }