fix(forward): make dnstap FORWARDER_* describe the socket from CoreDNS to upstream (#8184)

Signed-off-by: Jaime Hablutzel <hablutzel1@gmail.com>

plugin/forward/dnstap.go

@@ -7,7 +7,6 @@ import (
 	"time"
 
 	"github.com/coredns/coredns/plugin/dnstap/msg"
-	"github.com/coredns/coredns/plugin/pkg/proxy"
 	"github.com/coredns/coredns/request"
 
 	tap "github.com/dnstap/golang-dnstap"
@@ -15,7 +14,7 @@ import (
 )
 
 // toDnstap will send the forward and received message to the dnstap plugin.
-func toDnstap(ctx context.Context, f *Forward, host string, state request.Request, opts proxy.Options, reply *dns.Msg, start time.Time) {
+func toDnstap(ctx context.Context, f *Forward, host string, localAddr net.Addr, proto string, state request.Request, reply *dns.Msg, start time.Time) {
 	ap, _ := netip.ParseAddrPort(host) // this is preparsed and can't err here
 	ip := net.IP(ap.Addr().AsSlice())
 	port := int(ap.Port())
@@ -24,15 +23,7 @@ func toDnstap(ctx context.Context, f *Forward, host string, state request.Reques
 		IP:   ip,
 		Port: port,
 	}
-	t := state.Proto()
+	if proto == "tcp" {
-	switch {
-	case opts.ForceTCP:
-		t = "tcp"
-	case opts.PreferUDP:
-		t = "udp"
-	}
-
-	if t == "tcp" {
 		ta = &net.TCPAddr{IP: ip, Port: port}
 	}
 
@@ -40,9 +31,12 @@ func toDnstap(ctx context.Context, f *Forward, host string, state request.Reques
 		// Query
 		q := new(tap.Message)
 		msg.SetQueryTime(q, start)
-		// Forwarder dnstap messages are from the perspective of the downstream server
+		// Forwarder dnstap messages are from the perspective of the
-		// (upstream is the forward server)
+		// downstream DNS server (current CoreDNS) to an upstream DNS
-		msg.SetQueryAddress(q, state.W.RemoteAddr())
+		// server.
+		if localAddr != nil {
+			msg.SetQueryAddress(q, localAddr)
+		}
 		msg.SetResponseAddress(q, ta)
 		if t.IncludeRawMessage {
 			buf, _ := state.Req.Pack()
@@ -59,7 +53,9 @@ func toDnstap(ctx context.Context, f *Forward, host string, state request.Reques
 				r.ResponseMessage = buf
 			}
 			msg.SetQueryTime(r, start)
-			msg.SetQueryAddress(r, state.W.RemoteAddr())
+			if localAddr != nil {
+				msg.SetQueryAddress(r, localAddr)
+			}
 			msg.SetResponseAddress(r, ta)
 			msg.SetResponseTime(r, time.Now())
 			msg.SetType(r, tap.Message_FORWARDER_RESPONSE)

plugin/forward/forward.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"crypto/tls"
 	"errors"
+	"net"
 	"net/http"
 	"sync/atomic"
 	"time"
@@ -169,12 +170,14 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 
 		var (
 			ret           *dns.Msg
+			localAddr     net.Addr
+			upstreamProto string
 			err           error
 		)
 		opts := f.opts
 
 		for {
-			ret, err = proxy.Connect(ctx, state, opts)
+			ret, localAddr, upstreamProto, err = proxy.Connect(ctx, state, opts)
 
 			if err == proxyPkg.ErrCachedClosed { // Remote side closed conn, can only happen with TCP.
 				continue
@@ -192,7 +195,7 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 		}
 
 		if len(f.tapPlugins) != 0 {
-			toDnstap(ctx, f, proxy.Addr(), state, opts, ret, start)
+			toDnstap(ctx, f, proxy.Addr(), localAddr, upstreamProto, state, ret, start)
 		}
 
 		upstreamErr = err

plugin/pkg/proxy/connect.go

@@ -8,6 +8,8 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
+	"net/http/httptrace"
 	"strconv"
 	"strings"
 	"sync/atomic"
@@ -105,7 +107,7 @@ func (t *Transport) Dial(proto string) (*persistConn, bool, error) {
 	return &persistConn{c: conn, created: time.Now()}, false, err
 }
 
-func (p *Proxy) lookupDNS(_ctx context.Context, state request.Request, opts Options) (*dns.Msg, error) {
+func (p *Proxy) lookupDNS(_ctx context.Context, state request.Request, opts Options) (*dns.Msg, net.Addr, string, error) {
 	var proto string
 	switch {
 	case opts.ForceTCP: // TCP flag has precedence over UDP flag
@@ -118,9 +120,24 @@ func (p *Proxy) lookupDNS(_ctx context.Context, state request.Request, opts Opti
 
 	pc, cached, err := p.transport.Dial(proto)
 	if err != nil {
-		return nil, err
+		return nil, nil, proto, err
 	}
 
+	// Dial may have upgraded the transport (e.g. from udp to tcp for DoT),
+	// so report the transport the dialed connection actually uses, not the
+	// requested proto.
+	if p.transport.transportTypeFromConn(pc) == typeUDP {
+		proto = "udp"
+	} else {
+		proto = "tcp"
+	}
+
+	// localAddr is CoreDNS's own outbound address on the upstream socket.
+	// The forward plugin reports it as the dnstap query_address (the
+	// initiator) so that query_address and response_address describe the
+	// two ends of the same upstream connection.
+	localAddr := pc.c.LocalAddr()
+
 	// Set buffer size correctly for this client.
 	pc.c.UDPSize = max(uint16(state.Size()), 512) // #nosec G115 -- UDP size fits in uint16
 
@@ -135,9 +152,9 @@ func (p *Proxy) lookupDNS(_ctx context.Context, state request.Request, opts Opti
 	if err := pc.c.WriteMsg(state.Req); err != nil {
 		pc.c.Close() // not giving it back
 		if err == io.EOF && cached {
-			return nil, ErrCachedClosed
+			return nil, localAddr, proto, ErrCachedClosed
 		}
-		return nil, err
+		return nil, localAddr, proto, err
 	}
 
 	var ret *dns.Msg
@@ -159,13 +176,13 @@ func (p *Proxy) lookupDNS(_ctx context.Context, state request.Request, opts Opti
 
 			pc.c.Close() // not giving it back
 			if err == io.EOF && cached {
-				return nil, ErrCachedClosed
+				return nil, localAddr, proto, ErrCachedClosed
 			}
 			// recovery the origin Id after upstream.
 			if ret != nil {
 				ret.Id = originId
 			}
-			return ret, err
+			return ret, localAddr, proto, err
 		}
 		// drop out-of-order responses
 		if state.Req.Id == ret.Id {
@@ -174,48 +191,65 @@ func (p *Proxy) lookupDNS(_ctx context.Context, state request.Request, opts Opti
 	}
 	p.transport.Yield(pc)
 
-	return ret, nil
+	return ret, localAddr, proto, nil
 }
 
-func (p *Proxy) lookupDoH(ctx context.Context, state request.Request, _ Options) (*dns.Msg, error) {
+func (p *Proxy) lookupDoH(ctx context.Context, state request.Request, _ Options) (*dns.Msg, net.Addr, string, error) {
+	// DoH always runs over TCP (HTTPS), regardless of the downstream
+	// client's protocol.
+	const proto = "tcp"
+
+	var localAddr net.Addr
+	trace := &httptrace.ClientTrace{
+		GotConn: func(info httptrace.GotConnInfo) {
+			localAddr = info.Conn.LocalAddr()
+		},
+	}
+	ctx = httptrace.WithClientTrace(ctx, trace)
+
 	req, err := doh.NewRequestWithContext(ctx, p.dohMethod, p.addr, state.Req)
 	if err != nil {
-		return nil, err
+		return nil, nil, proto, err
 	}
 
 	resp, err := p.transport.httpClient.Do(req)
 	if err != nil {
-		return nil, err
+		return nil, localAddr, proto, err
 	}
 
 	// ResponseToMsg always closes the body via defer resp.Body.Close().
 	ret, err := doh.ResponseToMsg(resp)
 	if err != nil {
-		return nil, err
+		return nil, localAddr, proto, err
 	}
 
-	return ret, nil
+	return ret, localAddr, proto, nil
 }
 
-// Connect selects an upstream, sends the request and waits for a response.
+// Connect selects an upstream, sends the request and waits for a response. It
-func (p *Proxy) Connect(ctx context.Context, state request.Request, opts Options) (*dns.Msg, error) {
+// also returns CoreDNS's own outbound address on the upstream socket
+// (localAddr) and the transport proto ("udp" or "tcp") actually used to reach
+// the upstream.
+func (p *Proxy) Connect(ctx context.Context, state request.Request, opts Options) (*dns.Msg, net.Addr, string, error) {
 	start := time.Now()
 	originId := state.Req.Id
 
 	var (
 		ret       *dns.Msg
+		localAddr net.Addr
+		proto     string
 		err       error
 	)
 	switch p.protocol {
 	case transport.HTTPS:
-		ret, err = p.lookupDoH(ctx, state, opts)
+		ret, localAddr, proto, err = p.lookupDoH(ctx, state, opts)
 	case transport.DNS, transport.TLS:
-		ret, err = p.lookupDNS(ctx, state, opts)
+		ret, localAddr, proto, err = p.lookupDNS(ctx, state, opts)
 	default:
-		return nil, fmt.Errorf("transport %s not supported to proxy", p.protocol)
+		return nil, nil, "", fmt.Errorf("transport %s not supported to proxy", p.protocol)
 	}
 	if err != nil {
-		return nil, err
+		return nil, localAddr, proto, err
 	}
 
 	// recovery the origin Id after upstream.
@@ -228,7 +262,7 @@ func (p *Proxy) Connect(ctx context.Context, state request.Request, opts Options
 
 	requestDuration.WithLabelValues(p.proxyName, p.addr, rc).Observe(time.Since(start).Seconds())
 
-	return ret, nil
+	return ret, localAddr, proto, nil
 }
 
 const cumulativeAvgWeight = 4

plugin/pkg/proxy/connect_test.go

@@ -1,8 +1,18 @@
 package proxy
 
 import (
+	"context"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 	"time"
+
+	"github.com/coredns/coredns/plugin/pkg/doh"
+	"github.com/coredns/coredns/plugin/pkg/transport"
+	"github.com/coredns/coredns/plugin/test"
+	"github.com/coredns/coredns/request"
+
+	"github.com/miekg/dns"
 )
 
 const (
@@ -49,3 +59,50 @@ func TestDial_MultipleCallsAfterStop(t *testing.T) {
 		}
 	}
 }
+
+// TestConnectHTTPSReportsTCP verifies that Connect reports the transport it
+// actually used to reach the upstream. For an HTTPS upstream that is always
+// "tcp", even when the downstream client connected over UDP.
+func TestConnectHTTPSReportsTCP(t *testing.T) {
+	s := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		msg, err := doh.RequestToMsg(r)
+		if err != nil {
+			http.Error(w, err.Error(), http.StatusBadRequest)
+			return
+		}
+
+		ret := new(dns.Msg)
+		reply := ret.SetReply(msg)
+		reply.Answer = append(reply.Answer, test.A("example.org. IN A 127.0.0.1"))
+
+		buf, err := reply.Pack()
+		if err != nil {
+			http.Error(w, err.Error(), http.StatusInternalServerError)
+			return
+		}
+
+		w.Header().Set("Content-Type", doh.MimeType)
+		w.Write(buf)
+	}))
+	defer s.Close()
+
+	p := NewProxy("TestConnectHTTPSReportsTCP", s.URL, transport.HTTPS)
+	p.SetHTTPClient(s.Client())
+
+	m := new(dns.Msg)
+	m.SetQuestion("example.org.", dns.TypeA)
+
+	// The downstream client connected over UDP.
+	req := request.Request{W: &test.ResponseWriter{TCP: false}, Req: m}
+
+	resp, _, proto, err := p.Connect(context.Background(), req, Options{})
+	if err != nil {
+		t.Fatalf("Connect failed: %v", err)
+	}
+	if resp == nil {
+		t.Fatal("Expected response, got nil")
+	}
+	if proto != "tcp" {
+		t.Errorf("HTTPS upstream with a UDP downstream client: expected reported proto %q, got %q", "tcp", proto)
+	}
+}

plugin/pkg/proxy/proxy_test.go

@@ -36,7 +36,7 @@ func TestProxy(t *testing.T) {
 	rec := dnstest.NewRecorder(&test.ResponseWriter{})
 	req := request.Request{Req: m, W: rec}
 
-	resp, err := p.Connect(context.Background(), req, Options{PreferUDP: true})
+	resp, _, _, err := p.Connect(context.Background(), req, Options{PreferUDP: true})
 	if err != nil {
 		t.Errorf("Failed to connect to testdnsserver: %s", err)
 	}
@@ -67,7 +67,7 @@ func TestProxyTLSFail(t *testing.T) {
 	rec := dnstest.NewRecorder(&test.ResponseWriter{})
 	req := request.Request{Req: m, W: rec}
 
-	_, err := p.Connect(context.Background(), req, Options{})
+	_, _, _, err := p.Connect(context.Background(), req, Options{})
 	if err == nil {
 		t.Fatal("Expected *not* to receive reply, but got one")
 	}
@@ -121,7 +121,7 @@ func TestProtocolSelection(t *testing.T) {
 				Req: m,
 			}
 
-			resp, err := p.Connect(context.Background(), req, tc.opts)
+			resp, _, proto, err := p.Connect(context.Background(), req, tc.opts)
 			if err != nil {
 				t.Fatalf("Connect failed: %v", err)
 			}
@@ -133,6 +133,10 @@ func TestProtocolSelection(t *testing.T) {
 			if receivedProto != tc.expectedProto {
 				t.Errorf("Expected protocol %q, but server received %q", tc.expectedProto, receivedProto)
 			}
+
+			if proto != tc.expectedProto {
+				t.Errorf("Expected Connect to report proto %q, got %q", tc.expectedProto, proto)
+			}
 		})
 	}
 }
@@ -214,7 +218,7 @@ func TestCoreDNSOverflow(t *testing.T) {
 		recorder := dnstest.NewRecorder(&test.ResponseWriter{})
 		request := request.Request{Req: queryMsg, W: recorder}
 
-		response, err := p.Connect(context.Background(), request, options)
+		response, _, _, err := p.Connect(context.Background(), request, options)
 		if err != nil {
 			t.Errorf("Failed to connect to testdnsserver: %s", err)
 		}