diff --git a/upstream/dnscrypt.go b/upstream/dnscrypt.go index 3a06376c6..574597789 100644 --- a/upstream/dnscrypt.go +++ b/upstream/dnscrypt.go @@ -56,8 +56,8 @@ var _ Upstream = (*dnsCrypt)(nil) func (p *dnsCrypt) Address() string { return p.addr.String() } // Exchange implements the [Upstream] interface for *dnsCrypt. -func (p *dnsCrypt) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { - resp, err = p.exchangeDNSCrypt(m) +func (p *dnsCrypt) Exchange(req *dns.Msg) (resp *dns.Msg, err error) { + resp, err = p.exchangeDNSCrypt(req) if errors.Is(err, os.ErrDeadlineExceeded) || errors.Is(err, io.EOF) { // If request times out, it is possible that the server configuration // has been changed. It is safe to assume that the key was rotated, see @@ -68,7 +68,7 @@ func (p *dnsCrypt) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { return nil, err } - return p.exchangeDNSCrypt(m) + return p.exchangeDNSCrypt(req) } return resp, err @@ -80,7 +80,7 @@ func (p *dnsCrypt) Close() (err error) { } // exchangeDNSCrypt attempts to send the DNS query and returns the response. -func (p *dnsCrypt) exchangeDNSCrypt(m *dns.Msg) (resp *dns.Msg, err error) { +func (p *dnsCrypt) exchangeDNSCrypt(req *dns.Msg) (resp *dns.Msg, err error) { var client *dnscrypt.Client var resolverInfo *dnscrypt.ResolverInfo func() { @@ -108,9 +108,9 @@ func (p *dnsCrypt) exchangeDNSCrypt(m *dns.Msg) (resp *dns.Msg, err error) { // Go on. } - resp, err = client.Exchange(m, resolverInfo) + resp, err = client.Exchange(req, resolverInfo) if resp != nil && resp.Truncated { - q := &m.Question[0] + q := &req.Question[0] p.logger.Debug( "dnscrypt received truncated, falling back to tcp", "addr", p.addr, @@ -118,9 +118,9 @@ func (p *dnsCrypt) exchangeDNSCrypt(m *dns.Msg) (resp *dns.Msg, err error) { ) tcpClient := &dnscrypt.Client{Timeout: p.timeout, Net: networkTCP} - resp, err = tcpClient.Exchange(m, resolverInfo) + resp, err = tcpClient.Exchange(req, resolverInfo) } - if err == nil && resp != nil && resp.Id != m.Id { + if err == nil && resp != nil && resp.Id != req.Id { err = dns.ErrId } diff --git a/upstream/doh.go b/upstream/doh.go index 1863f83aa..9fd74ff48 100644 --- a/upstream/doh.go +++ b/upstream/doh.go @@ -16,6 +16,7 @@ import ( "github.com/AdguardTeam/dnsproxy/internal/bootstrap" "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/httphdr" "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/miekg/dns" "github.com/quic-go/quic-go" @@ -140,19 +141,22 @@ var _ Upstream = (*dnsOverHTTPS)(nil) // password, the password is replaced with "xxxxx". func (p *dnsOverHTTPS) Address() string { return p.addrRedacted } -// Exchange implements the Upstream interface for *dnsOverHTTPS. -func (p *dnsOverHTTPS) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { +// Exchange implements the [Upstream] interface for *dnsOverHTTPS. +func (p *dnsOverHTTPS) Exchange(req *dns.Msg) (resp *dns.Msg, err error) { // In order to maximize HTTP cache friendliness, DoH clients using media // formats that include the ID field from the DNS message header, such // as "application/dns-message", SHOULD use a DNS ID of 0 in every DNS // request. // // See https://www.rfc-editor.org/rfc/rfc8484.html. - id := m.Id - m.Id = 0 + + // TODO(e.burkov): Use some smarter cloning approach. + req = req.Copy() + id := req.Id + req.Id = 0 defer func() { // Restore the original ID to not break compatibility with proxies. - m.Id = id + req.Id = id if resp != nil { resp.Id = id } @@ -166,7 +170,7 @@ func (p *dnsOverHTTPS) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { } // Make the first attempt to send the DNS query. - resp, err = p.exchangeHTTPS(client, m) + resp, err = p.exchangeHTTPS(client, req) // Make up to 2 attempts to re-create the HTTP client and send the request // again. There are several cases (mostly, with QUIC) where this workaround @@ -179,7 +183,7 @@ func (p *dnsOverHTTPS) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { return nil, fmt.Errorf("failed to reset http client: %w", err) } - resp, err = p.exchangeHTTPS(client, m) + resp, err = p.exchangeHTTPS(client, req) } if err != nil { @@ -266,8 +270,10 @@ func (p *dnsOverHTTPS) exchangeHTTPSClient( return nil, fmt.Errorf("creating http request to %s: %w", p.addrRedacted, err) } - httpReq.Header.Set("Accept", "application/dns-message") - httpReq.Header.Set("User-Agent", "") + // Prevent the client from sending User-Agent header, see + // https://github.com/AdguardTeam/dnsproxy/issues/211. + httpReq.Header.Set(httphdr.UserAgent, "") + httpReq.Header.Set(httphdr.Accept, "application/dns-message") httpResp, err := client.Do(httpReq) if err != nil { diff --git a/upstream/doq.go b/upstream/doq.go index fffbad486..2091946c2 100644 --- a/upstream/doq.go +++ b/upstream/doq.go @@ -142,14 +142,20 @@ var _ Upstream = (*dnsOverQUIC)(nil) func (p *dnsOverQUIC) Address() string { return p.addr.String() } // Exchange implements the [Upstream] interface for *dnsOverQUIC. -func (p *dnsOverQUIC) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { +func (p *dnsOverQUIC) Exchange(req *dns.Msg) (resp *dns.Msg, err error) { // When sending queries over a QUIC connection, the DNS Message ID MUST be - // set to zero. - id := m.Id - m.Id = 0 + // set to 0. The stream mapping for DoQ allows for unambiguous correlation + // of queries and responses, so the Message ID field is not required. + // + // See https://www.rfc-editor.org/rfc/rfc9250#section-4.2.1. + + // TODO(e.burkov): Use some smarter cloning approach. + req = req.Copy() + id := req.Id + req.Id = 0 defer func() { // Restore the original ID to not break compatibility with proxies. - m.Id = id + req.Id = id if resp != nil { resp.Id = id } @@ -162,7 +168,7 @@ func (p *dnsOverQUIC) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { } // Make the first attempt to send the DNS query. - resp, err = p.exchangeQUIC(m, conn) + resp, err = p.exchangeQUIC(req, conn) // Failure to use a cached connection should be handled gracefully as this // connection could have been closed by the server or simply be broken due @@ -182,7 +188,7 @@ func (p *dnsOverQUIC) Exchange(m *dns.Msg) (resp *dns.Msg, err error) { } // Retry sending the request through the new connection. - resp, err = p.exchangeQUIC(m, conn) + resp, err = p.exchangeQUIC(req, conn) } if err != nil { diff --git a/upstream/dot.go b/upstream/dot.go index fd2f2af1d..0d7c3abb5 100644 --- a/upstream/dot.go +++ b/upstream/dot.go @@ -89,7 +89,7 @@ var _ Upstream = (*dnsOverTLS)(nil) func (p *dnsOverTLS) Address() string { return p.addr.String() } // Exchange implements the [Upstream] interface for *dnsOverTLS. -func (p *dnsOverTLS) Exchange(m *dns.Msg) (reply *dns.Msg, err error) { +func (p *dnsOverTLS) Exchange(req *dns.Msg) (reply *dns.Msg, err error) { h, err := p.getDialer() if err != nil { return nil, fmt.Errorf("getting conn to %s: %w", p.addr, err) @@ -100,7 +100,7 @@ func (p *dnsOverTLS) Exchange(m *dns.Msg) (reply *dns.Msg, err error) { return nil, fmt.Errorf("getting conn to %s: %w", p.addr, err) } - reply, err = p.exchangeWithConn(conn, m) + reply, err = p.exchangeWithConn(conn, req) if err != nil { // The pooled connection might have been closed already, see // https://github.com/AdguardTeam/dnsproxy/issues/3. The following @@ -120,7 +120,7 @@ func (p *dnsOverTLS) Exchange(m *dns.Msg) (reply *dns.Msg, err error) { ) } - reply, err = p.exchangeWithConn(conn, m) + reply, err = p.exchangeWithConn(conn, req) if err != nil { return reply, errors.WithDeferred(err, conn.Close()) } @@ -192,15 +192,15 @@ func (p *dnsOverTLS) putBack(conn net.Conn) { } // exchangeWithConn tries to exchange the query using conn. -func (p *dnsOverTLS) exchangeWithConn(conn net.Conn, m *dns.Msg) (reply *dns.Msg, err error) { +func (p *dnsOverTLS) exchangeWithConn(conn net.Conn, req *dns.Msg) (reply *dns.Msg, err error) { addr := p.Address() - logBegin(p.logger, addr, networkTCP, m) + logBegin(p.logger, addr, networkTCP, req) defer func() { logFinish(p.logger, addr, networkTCP, err) }() dnsConn := dns.Conn{Conn: conn} - err = dnsConn.WriteMsg(m) + err = dnsConn.WriteMsg(req) if err != nil { return nil, fmt.Errorf("sending request to %s: %w", addr, err) } @@ -208,7 +208,7 @@ func (p *dnsOverTLS) exchangeWithConn(conn net.Conn, m *dns.Msg) (reply *dns.Msg reply, err = dnsConn.ReadMsg() if err != nil { return nil, fmt.Errorf("reading response from %s: %w", addr, err) - } else if reply.Id != m.Id { + } else if reply.Id != req.Id { return reply, dns.ErrId } diff --git a/upstream/upstream.go b/upstream/upstream.go index 559d54366..7cd2e5e77 100644 --- a/upstream/upstream.go +++ b/upstream/upstream.go @@ -30,11 +30,13 @@ import ( // Upstream is an interface for a DNS resolver. type Upstream interface { - // Exchange sends the DNS query req to this upstream and returns the - // response that has been received or an error if something went wrong. + // Exchange sends req to this upstream and returns the response that has + // been received or an error if something went wrong. The implementations + // must not modify req. Exchange(req *dns.Msg) (resp *dns.Msg, err error) - // Address returns the address of the upstream DNS resolver. + // Address returns the human-readable address of the upstream DNS resolver. + // It may differ from what was passed to [AddressToUpstream]. Address() (addr string) // Closer used to close the upstreams properly. Exchange shouldn't be