diff --git a/plugin/forward/dnstap.go b/plugin/forward/dnstap.go index 293dc10cf..272524fcd 100644 --- a/plugin/forward/dnstap.go +++ b/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() - switch { - case opts.ForceTCP: - t = "tcp" - case opts.PreferUDP: - t = "udp" - } - - if t == "tcp" { + if proto == "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 - // (upstream is the forward server) - msg.SetQueryAddress(q, state.W.RemoteAddr()) + // Forwarder dnstap messages are from the perspective of the + // downstream DNS server (current CoreDNS) to an upstream DNS + // 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) diff --git a/plugin/forward/forward.go b/plugin/forward/forward.go index e97c4503c..fdb3e71fc 100644 --- a/plugin/forward/forward.go +++ b/plugin/forward/forward.go @@ -8,6 +8,7 @@ import ( "context" "crypto/tls" "errors" + "net" "net/http" "sync/atomic" "time" @@ -168,13 +169,15 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg }) var ( - ret *dns.Msg - err error + 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 diff --git a/plugin/pkg/proxy/connect.go b/plugin/pkg/proxy/connect.go index cf788fdd3..e340c784d 100644 --- a/plugin/pkg/proxy/connect.go +++ b/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. -func (p *Proxy) Connect(ctx context.Context, state request.Request, opts Options) (*dns.Msg, error) { +// Connect selects an upstream, sends the request and waits for a response. It +// 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 - err error + 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 diff --git a/plugin/pkg/proxy/connect_test.go b/plugin/pkg/proxy/connect_test.go index 1feb1584f..e0d813a0f 100644 --- a/plugin/pkg/proxy/connect_test.go +++ b/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) + } +} diff --git a/plugin/pkg/proxy/proxy_test.go b/plugin/pkg/proxy/proxy_test.go index afb66204b..424128452 100644 --- a/plugin/pkg/proxy/proxy_test.go +++ b/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) }