fix(netxlite/dns): more stricly mirror stdlib error strings (#513)

This diff attempts to modify the errors reported by our custom
resolver by matching more strings from the stdlib.

Part of https://github.com/ooni/probe/issues/1733 and diff has been
extracted from https://github.com/ooni/probe-cli/pull/506.

internal/netxlite/certifi.go

@@ -1,5 +1,5 @@
 // Code generated by go generate; DO NOT EDIT.
-// 2021-09-07 12:37:42.667616 +0200 CEST m=+1.355122543
+// 2021-09-27 15:57:57.877144 +0200 CEST m=+1.353309960
 // https://curl.haxx.se/ca/cacert.pem
 
 package netxlite

internal/netxlite/dnsx/decoder.go

@@ -1,34 +1,42 @@
 package dnsx
 
 import (
-	"errors"
-
 	"github.com/miekg/dns"
+	"github.com/ooni/probe-cli/v3/internal/netxlite/errorsx"
 )
 
-// The Decoder decodes a DNS reply into A or AAAA entries. It will use the
+// The Decoder decodes DNS replies.
-// provided qtype and only look for mathing entries. It will return error if
-// there are no entries for the requested qtype inside the reply.
 type Decoder interface {
-	Decode(qtype uint16, data []byte) ([]string, error)
+	// DecodeLookupHost decodes an A or AAAA reply.
+	DecodeLookupHost(qtype uint16, data []byte) ([]string, error)
 }
 
 // MiekgDecoder uses github.com/miekg/dns to implement the Decoder.
 type MiekgDecoder struct{}
 
-// Decode implements Decoder.Decode.
+func (d *MiekgDecoder) parseReply(data []byte) (*dns.Msg, error) {
-func (d *MiekgDecoder) Decode(qtype uint16, data []byte) ([]string, error) {
 	reply := new(dns.Msg)
 	if err := reply.Unpack(data); err != nil {
 		return nil, err
 	}
 	// TODO(bassosimone): map more errors to net.DNSError names
+	// TODO(bassosimone): add support for lame referral.
 	switch reply.Rcode {
 	case dns.RcodeSuccess:
+		return reply, nil
 	case dns.RcodeNameError:
-		return nil, errors.New("ooniresolver: no such host")
+		return nil, errorsx.ErrOODNSNoSuchHost
+	case dns.RcodeRefused:
+		return nil, errorsx.ErrOODNSRefused
 	default:
-		return nil, errors.New("ooniresolver: query failed")
+		return nil, errorsx.ErrOODNSMisbehaving
+	}
+}
+
+func (d *MiekgDecoder) DecodeLookupHost(qtype uint16, data []byte) ([]string, error) {
+	reply, err := d.parseReply(data)
+	if err != nil {
+		return nil, err
 	}
 	var addrs []string
 	for _, answer := range reply.Answer {
@@ -46,7 +54,7 @@ func (d *MiekgDecoder) Decode(qtype uint16, data []byte) ([]string, error) {
 		}
 	}
 	if len(addrs) <= 0 {
-		return nil, errors.New("ooniresolver: no response returned")
+		return nil, errorsx.ErrOODNSNoAnswer
 	}
 	return addrs, nil
 }

internal/netxlite/dnsx/decoder_test.go

@@ -1,16 +1,18 @@
 package dnsx
 
 import (
+	"errors"
 	"net"
 	"strings"
 	"testing"
 
 	"github.com/miekg/dns"
+	"github.com/ooni/probe-cli/v3/internal/netxlite/errorsx"
 )
 
 func TestDecoderUnpackError(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(dns.TypeA, nil)
+	data, err := d.DecodeLookupHost(dns.TypeA, nil)
 	if err == nil {
 		t.Fatal("expected an error here")
 	}
@@ -21,20 +23,20 @@ func TestDecoderUnpackError(t *testing.T) {
 
 func TestDecoderNXDOMAIN(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(dns.TypeA, genReplyError(t, dns.RcodeNameError))
+	data, err := d.DecodeLookupHost(dns.TypeA, genReplyError(t, dns.RcodeNameError))
 	if err == nil || !strings.HasSuffix(err.Error(), "no such host") {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if data != nil {
 		t.Fatal("expected nil data here")
 	}
 }
 
-func TestDecoderOtherError(t *testing.T) {
+func TestDecoderRefusedError(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(dns.TypeA, genReplyError(t, dns.RcodeRefused))
+	data, err := d.DecodeLookupHost(dns.TypeA, genReplyError(t, dns.RcodeRefused))
-	if err == nil || !strings.HasSuffix(err.Error(), "query failed") {
+	if !errors.Is(err, errorsx.ErrOODNSRefused) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if data != nil {
 		t.Fatal("expected nil data here")
@@ -43,9 +45,9 @@ func TestDecoderOtherError(t *testing.T) {
 
 func TestDecoderNoAddress(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(dns.TypeA, genReplySuccess(t, dns.TypeA))
+	data, err := d.DecodeLookupHost(dns.TypeA, genReplySuccess(t, dns.TypeA))
-	if err == nil || !strings.HasSuffix(err.Error(), "no response returned") {
+	if !errors.Is(err, errorsx.ErrOODNSNoAnswer) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if data != nil {
 		t.Fatal("expected nil data here")
@@ -54,7 +56,7 @@ func TestDecoderNoAddress(t *testing.T) {
 
 func TestDecoderDecodeA(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(
+	data, err := d.DecodeLookupHost(
 		dns.TypeA, genReplySuccess(t, dns.TypeA, "1.1.1.1", "8.8.8.8"))
 	if err != nil {
 		t.Fatal(err)
@@ -72,7 +74,7 @@ func TestDecoderDecodeA(t *testing.T) {
 
 func TestDecoderDecodeAAAA(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(
+	data, err := d.DecodeLookupHost(
 		dns.TypeAAAA, genReplySuccess(t, dns.TypeAAAA, "::1", "fe80::1"))
 	if err != nil {
 		t.Fatal(err)
@@ -90,10 +92,10 @@ func TestDecoderDecodeAAAA(t *testing.T) {
 
 func TestDecoderUnexpectedAReply(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(
+	data, err := d.DecodeLookupHost(
 		dns.TypeA, genReplySuccess(t, dns.TypeAAAA, "::1", "fe80::1"))
-	if err == nil || !strings.HasSuffix(err.Error(), "no response returned") {
+	if !errors.Is(err, errorsx.ErrOODNSNoAnswer) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if data != nil {
 		t.Fatal("expected nil data here")
@@ -102,10 +104,10 @@ func TestDecoderUnexpectedAReply(t *testing.T) {
 
 func TestDecoderUnexpectedAAAAReply(t *testing.T) {
 	d := &MiekgDecoder{}
-	data, err := d.Decode(
+	data, err := d.DecodeLookupHost(
 		dns.TypeAAAA, genReplySuccess(t, dns.TypeA, "1.1.1.1", "8.8.4.4."))
-	if err == nil || !strings.HasSuffix(err.Error(), "no response returned") {
+	if !errors.Is(err, errorsx.ErrOODNSNoAnswer) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if data != nil {
 		t.Fatal("expected nil data here")
@@ -179,3 +181,20 @@ func genReplySuccess(t *testing.T, qtype uint16, ips ...string) []byte {
 	}
 	return data
 }
+
+func TestParseReply(t *testing.T) {
+	d := &MiekgDecoder{}
+	msg := &dns.Msg{}
+	msg.Rcode = dns.RcodeFormatError // an rcode we don't handle
+	data, err := msg.Pack()
+	if err != nil {
+		t.Fatal(err)
+	}
+	reply, err := d.parseReply(data)
+	if !errors.Is(err, errorsx.ErrOODNSMisbehaving) { // catch all error
+		t.Fatal("not the error we expected", err)
+	}
+	if reply != nil {
+		t.Fatal("expected nil reply")
+	}
+}

internal/netxlite/dnsx/dnsoverhttps.go

@@ -27,14 +27,14 @@ type DNSOverHTTPS struct {
 
 // NewDNSOverHTTPS creates a new DNSOverHTTP instance from the
 // specified http.Client and URL, as a convenience.
-func NewDNSOverHTTPS(client *http.Client, URL string) *DNSOverHTTPS {
+func NewDNSOverHTTPS(client HTTPClient, URL string) *DNSOverHTTPS {
 	return NewDNSOverHTTPSWithHostOverride(client, URL, "")
 }
 
 // NewDNSOverHTTPSWithHostOverride is like NewDNSOverHTTPS except that
 // it's creating a resolver where we use the specified host.
 func NewDNSOverHTTPSWithHostOverride(
-	client *http.Client, URL, hostOverride string) *DNSOverHTTPS {
+	client HTTPClient, URL, hostOverride string) *DNSOverHTTPS {
 	return &DNSOverHTTPS{Client: client, URL: URL, HostOverride: hostOverride}
 }
 

internal/netxlite/dnsx/dnsoverhttps_test.go

@@ -175,3 +175,18 @@ func TestDNSOverHTTPSHostOverride(t *testing.T) {
 		t.Fatal("did not see correct host override")
 	}
 }
+
+func TestDNSOverHTTPSCloseIdleConnections(t *testing.T) {
+	var called bool
+	doh := &DNSOverHTTPS{
+		Client: &mocks.HTTPClient{
+			MockCloseIdleConnections: func() {
+				called = true
+			},
+		},
+	}
+	doh.CloseIdleConnections()
+	if !called {
+		t.Fatal("not called")
+	}
+}

internal/netxlite/dnsx/serial.go

@@ -51,8 +51,8 @@ func (r *SerialResolver) CloseIdleConnections() {
 // LookupHost implements Resolver.LookupHost.
 func (r *SerialResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	var addrs []string
-	addrsA, errA := r.roundTripWithRetry(ctx, hostname, dns.TypeA)
+	addrsA, errA := r.lookupHostWithRetry(ctx, hostname, dns.TypeA)
-	addrsAAAA, errAAAA := r.roundTripWithRetry(ctx, hostname, dns.TypeAAAA)
+	addrsAAAA, errAAAA := r.lookupHostWithRetry(ctx, hostname, dns.TypeAAAA)
 	if errA != nil && errAAAA != nil {
 		return nil, errA
 	}
@@ -61,11 +61,11 @@ func (r *SerialResolver) LookupHost(ctx context.Context, hostname string) ([]str
 	return addrs, nil
 }
 
-func (r *SerialResolver) roundTripWithRetry(
+func (r *SerialResolver) lookupHostWithRetry(
 	ctx context.Context, hostname string, qtype uint16) ([]string, error) {
 	var errorslist []error
 	for i := 0; i < 3; i++ {
-		replies, err := r.roundTrip(ctx, hostname, qtype)
+		replies, err := r.lookupHostWithoutRetry(ctx, hostname, qtype)
 		if err == nil {
 			return replies, nil
 		}
@@ -87,7 +87,9 @@ func (r *SerialResolver) roundTripWithRetry(
 	return nil, errorslist[0]
 }
 
-func (r *SerialResolver) roundTrip(
+// lookupHostWithoutRetry issues a lookup host query for the specified
+// qtype (dns.A or dns.AAAA) without retrying on failure.
+func (r *SerialResolver) lookupHostWithoutRetry(
 	ctx context.Context, hostname string, qtype uint16) ([]string, error) {
 	querydata, err := r.Encoder.Encode(hostname, qtype, r.Txp.RequiresPadding())
 	if err != nil {
@@ -97,5 +99,5 @@ func (r *SerialResolver) roundTrip(
 	if err != nil {
 		return nil, err
 	}
-	return r.Decoder.Decode(qtype, replydata)
+	return r.Decoder.DecodeLookupHost(qtype, replydata)
 }

internal/netxlite/dnsx/serial_test.go

@@ -5,7 +5,6 @@ import (
 	"crypto/tls"
 	"errors"
 	"net"
-	"strings"
 	"testing"
 
 	"github.com/miekg/dns"
@@ -79,8 +78,8 @@ func TestOONIWithEmptyReply(t *testing.T) {
 	}
 	r := NewSerialResolver(txp)
 	addrs, err := r.LookupHost(context.Background(), "www.gogle.com")
-	if err == nil || !strings.HasSuffix(err.Error(), "no response returned") {
+	if !errors.Is(err, errorsx.ErrOODNSNoAnswer) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if addrs != nil {
 		t.Fatal("expected nil address here")
@@ -146,3 +145,18 @@ func TestOONIWithTimeout(t *testing.T) {
 		t.Fatal("we didn't actually take the timeouts")
 	}
 }
+
+func TestSerialResolverCloseIdleConnections(t *testing.T) {
+	var called bool
+	r := &SerialResolver{
+		Txp: &mocks.RoundTripper{
+			MockCloseIdleConnections: func() {
+				called = true
+			},
+		},
+	}
+	r.CloseIdleConnections()
+	if !called {
+		t.Fatal("not called")
+	}
+}

internal/netxlite/errorsx/classify.go

@@ -84,12 +84,18 @@ func classifyWithStringSuffix(err error) string {
 	if strings.HasSuffix(s, "TLS handshake timeout") {
 		return FailureGenericTimeoutError
 	}
-	if strings.HasSuffix(s, "no such host") {
+	if strings.HasSuffix(s, DNSNoSuchHostSuffix) {
 		// This is dns_lookup_error in MK but such error is used as a
 		// generic "hey, the lookup failed" error. Instead, this error
 		// that we return here is significantly more specific.
 		return FailureDNSNXDOMAINError
 	}
+	if strings.HasSuffix(s, DNSServerMisbehavingSuffix) {
+		return FailureDNSServerMisbehaving
+	}
+	if strings.HasSuffix(s, DNSNoAnswerSuffix) {
+		return FailureDNSNoAnswer
+	}
 	if strings.HasSuffix(s, "use of closed network connection") {
 		return FailureConnectionAlreadyClosed
 	}
@@ -223,6 +229,21 @@ func quicIsCertificateError(alert uint8) bool {
 // filters for DNS bogons MUST use this error.
 var ErrDNSBogon = errors.New("dns: detected bogon address")
 
+// These strings are same as the standard library.
+const (
+	DNSNoSuchHostSuffix        = "no such host"
+	DNSServerMisbehavingSuffix = "server misbehaving"
+	DNSNoAnswerSuffix          = "no answer from DNS server"
+)
+
+// These errors are returned by the decoder and/or the serial resolver.
+var (
+	ErrOODNSNoSuchHost  = fmt.Errorf("ooniresolver: %s", DNSNoSuchHostSuffix)
+	ErrOODNSRefused     = errors.New("ooniresolver: refused")
+	ErrOODNSMisbehaving = fmt.Errorf("ooniresolver: %s", DNSServerMisbehavingSuffix)
+	ErrOODNSNoAnswer    = fmt.Errorf("ooniresolver: %s", DNSNoAnswerSuffix)
+)
+
 // ClassifyResolverError maps an error occurred during a domain name
 // resolution to the corresponding OONI failure string.
 //
@@ -239,6 +260,11 @@ func ClassifyResolverError(err error) string {
 	if errors.Is(err, ErrDNSBogon) {
 		return FailureDNSBogonError // not in MK
 	}
+	// Implementation note: we match errors that share the same
+	// string of the stdlib in the generic classifier.
+	if errors.Is(err, ErrOODNSRefused) {
+		return FailureDNSRefusedError // not in MK
+	}
 	return ClassifyGenericError(err)
 }
 

internal/netxlite/errorsx/classify_test.go

@@ -77,6 +77,18 @@ func TestClassifyGenericError(t *testing.T) {
 		}
 	})
 
+	t.Run("for dns server misbehaving", func(t *testing.T) {
+		if ClassifyGenericError(errors.New("dns server misbehaving")) != FailureDNSServerMisbehaving {
+			t.Fatal("unexpected results")
+		}
+	})
+
+	t.Run("for no answer from DNS server", func(t *testing.T) {
+		if ClassifyGenericError(errors.New("no answer from DNS server")) != FailureDNSNoAnswer {
+			t.Fatal("unexpected results")
+		}
+	})
+
 	t.Run("for use of closed network connection", func(t *testing.T) {
 		err := errors.New("read tcp 10.0.2.15:56948->93.184.216.34:443: use of closed network connection")
 		if ClassifyGenericError(err) != FailureConnectionAlreadyClosed {
@@ -251,6 +263,12 @@ func TestClassifyResolverError(t *testing.T) {
 		}
 	})
 
+	t.Run("for refused", func(t *testing.T) {
+		if ClassifyResolverError(ErrOODNSRefused) != FailureDNSRefusedError {
+			t.Fatal("unexpected result")
+		}
+	})
+
 	t.Run("for another kind of error", func(t *testing.T) {
 		if ClassifyResolverError(io.EOF) != FailureEOFError {
 			t.Fatal("unexpected result")

internal/netxlite/errorsx/errno.go

@@ -1,5 +1,5 @@
 // Code generated by go generate; DO NOT EDIT.
-// Generated: 2021-09-27 14:20:26.411929 +0200 CEST m=+0.233208042
+// Generated: 2021-09-27 15:57:58.500515 +0200 CEST m=+0.304199251
 
 package errorsx
 
@@ -50,6 +50,9 @@ const (
 	//
 	FailureDNSBogonError           = "dns_bogon_error"
 	FailureDNSNXDOMAINError        = "dns_nxdomain_error"
+	FailureDNSRefusedError         = "dns_refused_error"
+	FailureDNSServerMisbehaving    = "dns_server_misbehaving"
+	FailureDNSNoAnswer             = "dns_no_answer"
 	FailureEOFError                = "eof_error"
 	FailureGenericTimeoutError     = "generic_timeout_error"
 	FailureQUICIncompatibleVersion = "quic_incompatible_version"
@@ -94,6 +97,9 @@ var failuresMap = map[string]string{
 	"wrong_protocol_type":            "wrong_protocol_type",
 	"dns_bogon_error":                "dns_bogon_error",
 	"dns_nxdomain_error":             "dns_nxdomain_error",
+	"dns_refused_error":              "dns_refused_error",
+	"dns_server_misbehaving":         "dns_server_misbehaving",
+	"dns_no_answer":                  "dns_no_answer",
 	"eof_error":                      "eof_error",
 	"generic_timeout_error":          "generic_timeout_error",
 	"quic_incompatible_version":      "quic_incompatible_version",

internal/netxlite/errorsx/errno_test.go

@@ -1,5 +1,5 @@
 // Code generated by go generate; DO NOT EDIT.
-// Generated: 2021-09-27 14:20:26.459041 +0200 CEST m=+0.280321626
+// Generated: 2021-09-27 15:57:58.553101 +0200 CEST m=+0.356786917
 
 package errorsx
 

internal/netxlite/errorsx/errno_unix.go

@@ -1,5 +1,5 @@
 // Code generated by go generate; DO NOT EDIT.
-// Generated: 2021-09-27 14:20:26.179327 +0200 CEST m=+0.000601459
+// Generated: 2021-09-27 15:57:58.197535 +0200 CEST m=+0.001212709
 
 package errorsx
 

internal/netxlite/errorsx/errno_windows.go

@@ -1,5 +1,5 @@
 // Code generated by go generate; DO NOT EDIT.
-// Generated: 2021-09-27 14:20:26.380511 +0200 CEST m=+0.201789292
+// Generated: 2021-09-27 15:57:58.455744 +0200 CEST m=+0.259427834
 
 package errorsx
 

internal/netxlite/errorsx/internal/generrno/main.go

@@ -93,6 +93,9 @@ var Specs = []*ErrorSpec{
 	// we must write "DNS" rather than writing "dns".
 	NewLibraryError("DNS_bogon_error"),
 	NewLibraryError("DNS_NXDOMAIN_error"),
+	NewLibraryError("DNS_refused_error"),
+	NewLibraryError("DNS_server_misbehaving"),
+	NewLibraryError("DNS_no_answer"),
 	NewLibraryError("EOF_error"),
 	NewLibraryError("generic_timeout_error"),
 	NewLibraryError("QUIC_incompatible_version"),

internal/netxlite/resolver.go

@@ -3,6 +3,7 @@ package netxlite
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"time"
 
@@ -128,15 +129,16 @@ type resolverLogger struct {
 var _ Resolver = &resolverLogger{}
 
 func (r *resolverLogger) LookupHost(ctx context.Context, hostname string) ([]string, error) {
-	r.Logger.Debugf("resolve %s...", hostname)
+	prefix := fmt.Sprintf("resolve[A,AAAA] %s with %s (%s)", hostname, r.Network(), r.Address())
+	r.Logger.Debugf("%s...", prefix)
 	start := time.Now()
 	addrs, err := r.Resolver.LookupHost(ctx, hostname)
 	elapsed := time.Since(start)
 	if err != nil {
-		r.Logger.Debugf("resolve %s... %s in %s", hostname, err, elapsed)
+		r.Logger.Debugf("%s... %s in %s", prefix, err, elapsed)
 		return nil, err
 	}
-	r.Logger.Debugf("resolve %s... %+v in %s", hostname, addrs, elapsed)
+	r.Logger.Debugf("%s... %+v in %s", prefix, addrs, elapsed)
 	return addrs, nil
 }
 

internal/netxlite/resolver_test.go

@@ -146,12 +146,18 @@ func TestResolverLogger(t *testing.T) {
 				},
 			}
 			expected := []string{"1.1.1.1"}
-			r := resolverLogger{
+			r := &resolverLogger{
 				Logger: lo,
 				Resolver: &mocks.Resolver{
 					MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
 						return expected, nil
 					},
+					MockNetwork: func() string {
+						return "system"
+					},
+					MockAddress: func() string {
+						return ""
+					},
 				},
 			}
 			addrs, err := r.LookupHost(context.Background(), "dns.google")
@@ -174,12 +180,18 @@ func TestResolverLogger(t *testing.T) {
 				},
 			}
 			expected := errors.New("mocked error")
-			r := resolverLogger{
+			r := &resolverLogger{
 				Logger: lo,
 				Resolver: &mocks.Resolver{
 					MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
 						return nil, expected
 					},
+					MockNetwork: func() string {
+						return "system"
+					},
+					MockAddress: func() string {
+						return ""
+					},
 				},
 			}
 			addrs, err := r.LookupHost(context.Background(), "dns.google")

internal/netxlite/tls.go

@@ -121,7 +121,7 @@ type TLSHandshaker interface {
 	//
 	// QUIRK: The returned connection will always implement the TLSConn interface
 	// exposed by this package. A future version of this interface will instead
-	// return directly a TLSConn and remove the ConnectionState param.
+	// return directly a TLSConn to avoid unconditional castings.
 	Handshake(ctx context.Context, conn net.Conn, config *tls.Config) (
 		net.Conn, tls.ConnectionState, error)
 }