Pull request: Fix #283: handle more edge cases for DoH3 retries

Merge in DNS/dnsproxy from fix-283-doh3 to master

Squashed commit of the following:

commit e2437d0
Author: Andrey Meshkov <[email protected]>
Date:   Wed Oct 19 13:31:46 2022 +0300

    added more retry cases for quic

commit 4bf6980
Author: Andrey Meshkov <[email protected]>
Date:   Wed Oct 19 12:38:08 2022 +0300

    fix review comments

commit 25c0c94
Author: Andrey Meshkov <[email protected]>
Date:   Wed Oct 19 12:15:38 2022 +0300

    Fix #283: hanlde more edge cases for DoH3 retries

proxy/server_quic.go

@@ -396,7 +396,8 @@ func newQUICAddrValidator(cacheSize int, ttl time.Duration) (v *quicAddrValidato
 // client. This allows the server to verify the client's address but increases
 // the latency.
 func (v *quicAddrValidator) requiresValidation(addr net.Addr) (ok bool) {
-	key := addr.String()
+	// addr must be *net.UDPAddr here and if it's not we don't mind panic.
+	key := addr.(*net.UDPAddr).IP.String()
 	if v.cache.Has(key) {
 		return false
 	}

upstream/upstream_doh.go

@@ -48,8 +48,8 @@ type dnsOverHTTPS struct {
 	// The Client's Transport typically has internal state (cached TCP
 	// connections), so Clients should be reused instead of created as
 	// needed. Clients are safe for concurrent use by multiple goroutines.
-	client      *http.Client
-	clientGuard sync.Mutex
+	client   *http.Client
+	clientMu sync.Mutex
 
 	// quicConfig is the QUIC configuration that is used if HTTP/3 is enabled
 	// for this upstream.
@@ -106,71 +106,79 @@ func (p *dnsOverHTTPS) Exchange(m *dns.Msg) (resp *dns.Msg, err error) {
 
 	// Check if there was already an active client before sending the request.
 	// We'll only attempt to re-connect if there was one.
-	hasClient := p.hasClient()
+	client, isCached, err := p.getClient()
+	if err != nil {
+		return nil, fmt.Errorf("failed to init http client: %w", err)
+	}
 
 	// Make the first attempt to send the DNS query.
-	resp, err = p.exchangeHTTPS(m)
+	resp, err = p.exchangeHTTPS(client, m)
 
 	// 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
 	// is necessary to make HTTP client usable.  We need to make 2 attempts in
 	// the case when the connection was closed (due to inactivity for example)
 	// AND the server refuses to open a 0-RTT connection.
-	for i := 0; hasClient && p.shouldRetry(err) && i < 2; i++ {
-		log.Debug("re-creating the HTTP client and retrying due to %v", err)
-
-		// Make sure we reset the client here.
-		p.resetClient(err)
+	for i := 0; isCached && p.shouldRetry(err) && i < 2; i++ {
+		client, err = p.resetClient(err)
+		if err != nil {
+			return nil, fmt.Errorf("failed to reset http client: %w", err)
+		}
 
-		resp, err = p.exchangeHTTPS(m)
+		resp, err = p.exchangeHTTPS(client, m)
 	}
 
 	if err != nil {
 		// If the request failed anyway, make sure we don't use this client.
-		p.resetClient(err)
+		_, resErr := p.resetClient(err)
+
+		return nil, errors.WithDeferred(err, resErr)
 	}
 
 	return resp, err
 }
 
 // Close implements the Upstream interface for *dnsOverHTTPS.
 func (p *dnsOverHTTPS) Close() (err error) {
-	p.clientGuard.Lock()
-	defer p.clientGuard.Unlock()
+	p.clientMu.Lock()
+	defer p.clientMu.Unlock()
 
 	runtime.SetFinalizer(p, nil)
 
 	if p.client == nil {
 		return nil
 	}
 
-	// We should only explicitly close it when the client is for DoH3.  Native
-	// http.Client is stateless and does not require explicit cleanup.
-	if t, ok := p.client.Transport.(*http3.RoundTripper); ok {
-		err = t.Close()
-	}
-
-	return err
+	return p.closeClient(p.client)
 }
 
-// exchangeHTTPS creates an HTTP client and sends the DNS query using it.
-func (p *dnsOverHTTPS) exchangeHTTPS(m *dns.Msg) (resp *dns.Msg, err error) {
-	client, err := p.getClient()
-	if err != nil {
-		return nil, fmt.Errorf("initializing http client: %w", err)
+// closeClient cleans up resources used by client if necessary.  Note, that at
+// this point it should only be done for HTTP/3 as it may leak due to keep-alive
+// connections.
+func (p *dnsOverHTTPS) closeClient(client *http.Client) (err error) {
+	if t, ok := client.Transport.(*http3.RoundTripper); ok {
+		return t.Close()
 	}
 
-	logBegin(p.Address(), m)
-	resp, err = p.exchangeHTTPSClient(m, client)
+	return nil
+}
+
+// exchangeHTTPS logs the request and its result and calls exchangeHTTPSClient.
+func (p *dnsOverHTTPS) exchangeHTTPS(client *http.Client, req *dns.Msg) (resp *dns.Msg, err error) {
+	logBegin(p.Address(), req)
+	resp, err = p.exchangeHTTPSClient(client, req)
 	logFinish(p.Address(), err)
 
 	return resp, err
 }
 
 // exchangeHTTPSClient sends the DNS query to a DoH resolver using the specified
 // http.Client instance.
-func (p *dnsOverHTTPS) exchangeHTTPSClient(m *dns.Msg, client *http.Client) (*dns.Msg, error) {
-	buf, err := m.Pack()
+func (p *dnsOverHTTPS) exchangeHTTPSClient(
+	client *http.Client,
+	req *dns.Msg,
+) (resp *dns.Msg, err error) {
+	buf, err := req.Pack()
 	if err != nil {
 		return nil, fmt.Errorf("packing message: %w", err)
 	}
@@ -190,50 +198,51 @@ func (p *dnsOverHTTPS) exchangeHTTPSClient(m *dns.Msg, client *http.Client) (*dn
 		RawQuery: fmt.Sprintf("dns=%s", base64.RawURLEncoding.EncodeToString(buf)),
 	}
 
-	req, err := http.NewRequest(method, u.String(), nil)
+	httpReq, err := http.NewRequest(method, u.String(), nil)
 	if err != nil {
 		return nil, fmt.Errorf("creating http request to %s: %w", p.boot.URL, err)
 	}
 
-	req.Header.Set("Accept", "application/dns-message")
-	req.Header.Set("User-Agent", "")
+	httpReq.Header.Set("Accept", "application/dns-message")
+	httpReq.Header.Set("User-Agent", "")
 
-	resp, err := client.Do(req)
-	if resp != nil && resp.Body != nil {
-		defer resp.Body.Close()
-	}
+	httpResp, err := client.Do(httpReq)
 	if err != nil {
 		return nil, fmt.Errorf("requesting %s: %w", p.boot.URL, err)
 	}
+	defer httpResp.Body.Close()
 
-	body, err := io.ReadAll(resp.Body)
+	body, err := io.ReadAll(httpResp.Body)
 	if err != nil {
 		return nil, fmt.Errorf("reading %s: %w", p.boot.URL, err)
 	}
 
-	if resp.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("got status code %d from %s", resp.StatusCode, p.boot.URL)
+	if httpResp.StatusCode != http.StatusOK {
+		return nil,
+			fmt.Errorf(
+				"expected status %d, got %d from %s",
+				http.StatusOK,
+				httpResp.StatusCode,
+				p.boot.URL,
+			)
 	}
 
-	response := dns.Msg{}
-	err = response.Unpack(body)
+	resp = &dns.Msg{}
+	err = resp.Unpack(body)
 	if err != nil {
-		return nil, fmt.Errorf("unpacking response from %s: body is %s: %w", p.boot.URL, string(body), err)
+		return nil, fmt.Errorf(
+			"unpacking response from %s: body is %s: %w",
+			p.boot.URL,
+			body,
+			err,
+		)
 	}
 
-	if response.Id != m.Id {
+	if resp.Id != req.Id {
 		err = dns.ErrId
 	}
 
-	return &response, err
-}
-
-// hasClient returns true if this connection already has an active HTTP client.
-func (p *dnsOverHTTPS) hasClient() (ok bool) {
-	p.clientGuard.Lock()
-	defer p.clientGuard.Unlock()
-
-	return p.client != nil
+	return resp, err
 }
 
 // shouldRetry checks what error we have received and returns true if we should
@@ -263,16 +272,27 @@ func (p *dnsOverHTTPS) shouldRetry(err error) (ok bool) {
 // resetClient triggers re-creation of the *http.Client that is used by this
 // upstream.  This method accepts the error that caused resetting client as
 // depending on the error we may also reset the QUIC config.
-func (p *dnsOverHTTPS) resetClient(err error) {
-	p.clientGuard.Lock()
-	defer p.clientGuard.Unlock()
+func (p *dnsOverHTTPS) resetClient(resetErr error) (client *http.Client, err error) {
+	p.clientMu.Lock()
+	defer p.clientMu.Unlock()
 
-	p.client = nil
-
-	if errors.Is(err, quic.Err0RTTRejected) {
+	if errors.Is(resetErr, quic.Err0RTTRejected) {
 		// Reset the TokenStore only if 0-RTT was rejected.
 		p.resetQUICConfig()
 	}
+
+	oldClient := p.client
+	if oldClient != nil {
+		closeErr := p.closeClient(oldClient)
+		if closeErr != nil {
+			log.Info("warning: failed to close the old http client: %v", closeErr)
+		}
+	}
+
+	log.Debug("re-creating the http client due to %v", resetErr)
+	p.client, err = p.createClient()
+
+	return p.client, err
 }
 
 // getQUICConfig returns the QUIC config in a thread-safe manner.  Note, that
@@ -296,25 +316,26 @@ func (p *dnsOverHTTPS) resetQUICConfig() {
 
 // getClient gets or lazily initializes an HTTP client (and transport) that will
 // be used for this DoH resolver.
-func (p *dnsOverHTTPS) getClient() (c *http.Client, err error) {
+func (p *dnsOverHTTPS) getClient() (c *http.Client, isCached bool, err error) {
 	startTime := time.Now()
 
-	p.clientGuard.Lock()
-	defer p.clientGuard.Unlock()
+	p.clientMu.Lock()
+	defer p.clientMu.Unlock()
 	if p.client != nil {
-		return p.client, nil
+		return p.client, true, nil
 	}
 
 	// Timeout can be exceeded while waiting for the lock. This happens quite
 	// often on mobile devices.
 	elapsed := time.Since(startTime)
 	if p.boot.options.Timeout > 0 && elapsed > p.boot.options.Timeout {
-		return nil, fmt.Errorf("timeout exceeded: %s", elapsed)
+		return nil, false, fmt.Errorf("timeout exceeded: %s", elapsed)
 	}
 
+	log.Debug("creating a new http client")
 	p.client, err = p.createClient()
 
-	return p.client, err
+	return p.client, false, err
 }
 
 // createClient creates a new *http.Client instance.  The HTTP protocol version

upstream/upstream_quic.go

@@ -412,6 +412,25 @@ func isQUICRetryError(err error) (ok bool) {
 		return true
 	}
 
+	var resetErr *quic.StatelessResetError
+	if errors.As(err, &resetErr) {
+		// A stateless reset is sent when a server receives a QUIC packet that
+		// it doesn't know how to decrypt.  For instance, it may happen when
+		// the server was recently rebooted.  We should reconnect and try again
+		// in this case.
+		return true
+	}
+
+	var qTransportError *quic.TransportError
+	if errors.As(err, &qTransportError) && qTransportError.ErrorCode == quic.NoError {
+		// A transport error with the NO_ERROR error code could be sent by the
+		// server when it considers that it's time to close the connection.
+		// For example, Google DNS eventually closes an active connection with
+		// the NO_ERROR code and "Connection max age expired" message:
+		// https://github.com/AdguardTeam/dnsproxy/issues/283
+		return true
+	}
+
 	if errors.Is(err, quic.Err0RTTRejected) {
 		// This error happens when we try to establish a 0-RTT connection with
 		// a token the server is no more aware of.  This can be reproduced by