From 17335d21049c724e365d4e9879286cd2fdb9aba5 Mon Sep 17 00:00:00 2001 From: Nick Hill Date: Fri, 4 Oct 2024 12:23:34 -0500 Subject: [PATCH] net/dns/resolver: forward SERVFAIL responses over PeerDNS As per the docstring, (*forwarder).forwardWithDestChan should either send to responseChan and returns nil, or returns a non-nil error (without sending to the channel). However, this does not hold when all upstream DNS servers replied with an error. We've been handling this special error path in (*Resolver).Query but not in (*Resolver).HandlePeerDNSQuery. As a result, SERVFAIL responses from upstream servers were being converted into HTTP 503 responses, instead of being properly forwarded as SERVFAIL within a successful HTTP response, as per RFC 8484, section 4.2.1: A successful HTTP response with a 2xx status code (see Section 6.3 of [RFC7231]) is used for any valid DNS response, regardless of the DNS response code. For example, a successful 2xx HTTP status code is used even with a DNS message whose DNS response code indicates failure, such as SERVFAIL or NXDOMAIN. In this PR we fix (*forwarder).forwardWithDestChan to no longer return an error when it sends a response to responseChan, and remove the special handling in (*Resolver).Query, as it is no longer necessary. Updates #13571 Signed-off-by: Nick Hill --- net/dns/resolver/forwarder.go | 1 + net/dns/resolver/tsdns.go | 10 +--------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/net/dns/resolver/forwarder.go b/net/dns/resolver/forwarder.go index 846ca3d5e..5920b7f29 100644 --- a/net/dns/resolver/forwarder.go +++ b/net/dns/resolver/forwarder.go @@ -1053,6 +1053,7 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo if verboseDNSForward() { f.logf("forwarder response(%d, %v, %d) = %d, %v", fq.txid, typ, len(domain), len(res.bs), firstErr) } + return nil } } return firstErr diff --git a/net/dns/resolver/tsdns.go b/net/dns/resolver/tsdns.go index d196ad4d6..43ba0acf1 100644 --- a/net/dns/resolver/tsdns.go +++ b/net/dns/resolver/tsdns.go @@ -321,15 +321,7 @@ func (r *Resolver) Query(ctx context.Context, bs []byte, family string, from net defer cancel() err = r.forwarder.forwardWithDestChan(ctx, packet{bs, family, from}, responses) if err != nil { - select { - // Best effort: use any error response sent by forwardWithDestChan. - // This is present in some errors paths, such as when all upstream - // DNS servers replied with an error. - case resp := <-responses: - return resp.bs, err - default: - return nil, err - } + return nil, err } return (<-responses).bs, nil }