From 080abf90d930c8a5a1a69b62b56907c27f75f558 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 23 Aug 2022 13:53:08 +0200 Subject: [PATCH] feat(dnsovergetaddrinfo): collect the CNAME (#876) * feat(dnsovergetaddrinfo): collect the CNAME This diff modifies how dnsovergetaddrinfo.go works such that the returned DNSResponse includes the CNAME. Closes https://github.com/ooni/probe/issues/2226. While there, recognize that we can remove getaddrinfoLookupHost and always call getaddrinfoLookupANY everywhere. (This simplification is why we did https://github.com/ooni/probe-cli/pull/874.) * fix: extra debugging because of failing CI Everything is OK locally (on macOS). However, maybe things are a bit different on GNU/Linux perhaps? Here's the error: ``` --- FAIL: TestPass (0.11s) resolver_test.go:113: unexpected rcode FAIL coverage: 95.7% of statements FAIL github.com/ooni/probe-cli/v3/internal/cmd/jafar/resolver 0.242s ``` I'm a bit confused because jafar's resolver is _unrelated_. But actually this error never occurred again after a committed the debugging diff. --- internal/cmd/jafar/resolver/resolver_test.go | 2 +- internal/netxlite/dnsovergetaddrinfo.go | 47 +++++++++++++------- internal/netxlite/dnsovergetaddrinfo_test.go | 20 ++++----- internal/netxlite/getaddrinfo.go | 17 +------ internal/netxlite/getaddrinfo_cgo.go | 5 ++- internal/netxlite/getaddrinfo_test.go | 4 +- 6 files changed, 49 insertions(+), 46 deletions(-) diff --git a/internal/cmd/jafar/resolver/resolver_test.go b/internal/cmd/jafar/resolver/resolver_test.go index f6f05b7..103344d 100644 --- a/internal/cmd/jafar/resolver/resolver_test.go +++ b/internal/cmd/jafar/resolver/resolver_test.go @@ -110,7 +110,7 @@ func checkrequest( func checksuccess(t *testing.T, reply *dns.Msg) { if reply.Rcode != dns.RcodeSuccess { - t.Fatal("unexpected rcode") + t.Fatal("unexpected rcode", reply.Rcode) } if len(reply.Answer) < 1 { t.Fatal("too few answers") diff --git a/internal/netxlite/dnsovergetaddrinfo.go b/internal/netxlite/dnsovergetaddrinfo.go index c9b3897..d55125c 100644 --- a/internal/netxlite/dnsovergetaddrinfo.go +++ b/internal/netxlite/dnsovergetaddrinfo.go @@ -17,8 +17,11 @@ import ( // dnsOverGetaddrinfoTransport is a DNSTransport using getaddrinfo. type dnsOverGetaddrinfoTransport struct { - testableTimeout time.Duration - testableLookupHost func(ctx context.Context, domain string) ([]string, error) + // (OPTIONAL) allows to run tests with a short timeout + testableTimeout time.Duration + + // (OPTIONAL) allows to mock the underlying getaddrinfo call + testableLookupANY func(ctx context.Context, domain string) ([]string, string, error) } var _ model.DNSTransport = &dnsOverGetaddrinfoTransport{} @@ -28,13 +31,13 @@ func (txp *dnsOverGetaddrinfoTransport) RoundTrip( if query.Type() != dns.TypeANY { return nil, ErrNoDNSTransport } - addrs, err := txp.lookup(ctx, query.Domain()) + addrs, cname, err := txp.lookup(ctx, query.Domain()) if err != nil { return nil, err } resp := &dnsOverGetaddrinfoResponse{ addrs: addrs, - cname: "", // TODO: implement this functionality + cname: cname, query: query, } return resp, nil @@ -46,30 +49,42 @@ type dnsOverGetaddrinfoResponse struct { query model.DNSQuery } +// Used to move addrs and cname out of the worker goroutine +type dnsOverGetaddrinfoAddrsAndCNAME struct { + // List of resolved addresses (it's a bug if this is empty) + addrs []string + + // Resolved CNAME or empty string + cname string +} + func (txp *dnsOverGetaddrinfoTransport) lookup( - ctx context.Context, hostname string) ([]string, error) { + ctx context.Context, hostname string) ([]string, string, error) { // This code forces adding a shorter timeout to the domain name // resolutions when using the system resolver. We have seen cases // in which such a timeout becomes too large. One such case is // described in https://github.com/ooni/probe/issues/1726. - addrsch, errch := make(chan []string, 1), make(chan error, 1) + outch, errch := make(chan *dnsOverGetaddrinfoAddrsAndCNAME, 1), make(chan error, 1) ctx, cancel := context.WithTimeout(ctx, txp.timeout()) defer cancel() go func() { - addrs, err := txp.lookupfn()(ctx, hostname) + addrs, cname, err := txp.lookupfn()(ctx, hostname) if err != nil { errch <- err return } - addrsch <- addrs + outch <- &dnsOverGetaddrinfoAddrsAndCNAME{ + addrs: addrs, + cname: cname, + } }() select { case <-ctx.Done(): - return nil, ctx.Err() - case addrs := <-addrsch: - return addrs, nil + return nil, "", ctx.Err() + case p := <-outch: + return p.addrs, p.cname, nil case err := <-errch: - return nil, err + return nil, "", err } } @@ -80,11 +95,11 @@ func (txp *dnsOverGetaddrinfoTransport) timeout() time.Duration { return 15 * time.Second } -func (txp *dnsOverGetaddrinfoTransport) lookupfn() func(ctx context.Context, domain string) ([]string, error) { - if txp.testableLookupHost != nil { - return txp.testableLookupHost +func (txp *dnsOverGetaddrinfoTransport) lookupfn() func(ctx context.Context, domain string) ([]string, string, error) { + if txp.testableLookupANY != nil { + return txp.testableLookupANY } - return getaddrinfoLookupHost + return getaddrinfoLookupANY } func (txp *dnsOverGetaddrinfoTransport) RequiresPadding() bool { diff --git a/internal/netxlite/dnsovergetaddrinfo_test.go b/internal/netxlite/dnsovergetaddrinfo_test.go index bcb1b9e..fb4e341 100644 --- a/internal/netxlite/dnsovergetaddrinfo_test.go +++ b/internal/netxlite/dnsovergetaddrinfo_test.go @@ -57,8 +57,8 @@ func TestDNSOverGetaddrinfo(t *testing.T) { t.Run("RoundTrip", func(t *testing.T) { t.Run("with invalid query type", func(t *testing.T) { txp := &dnsOverGetaddrinfoTransport{ - testableLookupHost: func(ctx context.Context, domain string) ([]string, error) { - return []string{"8.8.8.8"}, nil + testableLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + return []string{"8.8.8.8"}, "dns.google", nil }, } encoder := &DNSEncoderMiekg{} @@ -75,8 +75,8 @@ func TestDNSOverGetaddrinfo(t *testing.T) { t.Run("with success", func(t *testing.T) { txp := &dnsOverGetaddrinfoTransport{ - testableLookupHost: func(ctx context.Context, domain string) ([]string, error) { - return []string{"8.8.8.8"}, nil + testableLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + return []string{"8.8.8.8"}, "dns.google", nil }, } encoder := &DNSEncoderMiekg{} @@ -124,10 +124,10 @@ func TestDNSOverGetaddrinfo(t *testing.T) { done := make(chan interface{}) txp := &dnsOverGetaddrinfoTransport{ testableTimeout: 1 * time.Microsecond, - testableLookupHost: func(ctx context.Context, domain string) ([]string, error) { + testableLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { defer wg.Done() <-done - return []string{"8.8.8.8"}, nil + return []string{"8.8.8.8"}, "dns.google", nil }, } encoder := &DNSEncoderMiekg{} @@ -150,10 +150,10 @@ func TestDNSOverGetaddrinfo(t *testing.T) { done := make(chan interface{}) txp := &dnsOverGetaddrinfoTransport{ testableTimeout: 1 * time.Microsecond, - testableLookupHost: func(ctx context.Context, domain string) ([]string, error) { + testableLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { defer wg.Done() <-done - return nil, errors.New("no such host") + return nil, "", errors.New("no such host") }, } encoder := &DNSEncoderMiekg{} @@ -172,8 +172,8 @@ func TestDNSOverGetaddrinfo(t *testing.T) { t.Run("with NXDOMAIN", func(t *testing.T) { txp := &dnsOverGetaddrinfoTransport{ - testableLookupHost: func(ctx context.Context, domain string) ([]string, error) { - return nil, ErrOODNSNoSuchHost + testableLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + return nil, "", ErrOODNSNoSuchHost }, } encoder := &DNSEncoderMiekg{} diff --git a/internal/netxlite/getaddrinfo.go b/internal/netxlite/getaddrinfo.go index e8318cb..1d67457 100644 --- a/internal/netxlite/getaddrinfo.go +++ b/internal/netxlite/getaddrinfo.go @@ -1,21 +1,6 @@ package netxlite -import ( - "context" - "errors" -) - -// getaddrinfoLookupHost performs a DNS lookup and returns the -// results. If we were compiled with CGO_ENABLED=0, then this -// function calls net.DefaultResolver.LookupHost. Otherwise, -// we call getaddrinfo. In such a case, if getaddrinfo returns a nonzero -// return value, we'll return as error an instance of the -// ErrGetaddrinfo error. This error will contain the specific -// code returned by getaddrinfo in its .Code field. -func getaddrinfoLookupHost(ctx context.Context, domain string) ([]string, error) { - addrs, _, err := getaddrinfoLookupANY(ctx, domain) - return addrs, err -} +import "errors" // ErrGetaddrinfo represents a getaddrinfo failure. type ErrGetaddrinfo struct { diff --git a/internal/netxlite/getaddrinfo_cgo.go b/internal/netxlite/getaddrinfo_cgo.go index f061d0d..91b5c0f 100644 --- a/internal/netxlite/getaddrinfo_cgo.go +++ b/internal/netxlite/getaddrinfo_cgo.go @@ -48,7 +48,10 @@ func getaddrinfoResolverNetwork() string { // the error that occurred. On error, the list of addresses is empty. The // CNAME may be empty on success, if there's no CNAME, but may also be // non-empty on failure, if the lookup result included a CNAME answer but -// did not include any A or AAAA answers. +// did not include any A or AAAA answers. If getaddrinfo returns a nonzero +// return value, we'll return as error an instance of the +// ErrGetaddrinfo error. This error will contain the specific +// code returned by getaddrinfo in its .Code field. func getaddrinfoLookupANY(ctx context.Context, domain string) ([]string, string, error) { return getaddrinfoStateSingleton.LookupANY(ctx, domain) } diff --git a/internal/netxlite/getaddrinfo_test.go b/internal/netxlite/getaddrinfo_test.go index b954fa1..a763741 100644 --- a/internal/netxlite/getaddrinfo_test.go +++ b/internal/netxlite/getaddrinfo_test.go @@ -7,8 +7,8 @@ import ( "testing" ) -func TestGetaddrinfoLookupHost(t *testing.T) { - addrs, err := getaddrinfoLookupHost(context.Background(), "127.0.0.1") +func TestGetaddrinfoLookupANY(t *testing.T) { + addrs, _, err := getaddrinfoLookupANY(context.Background(), "127.0.0.1") if err != nil { t.Fatal(err) }