From 7c86d8f7977979b13ae85080aec073e80e9daf4d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 17:50:09 +0100 Subject: [PATCH 1/9] cleanup(sessionresolver): don't depend on netx This diff closes https://github.com/ooni/probe/issues/2121 because it removes the last unnecessary netx usage. All the other packages that currently use netx are network experiments. We will eventually convert them to not use netx as part of other tracked issues. This diff closes https://github.com/ooni/probe/issues/2135 because now the sessionresolver does not depend on netx anymore. We refactor the way in which we conditionally configure byte counting for HTTP such that we use a free function rather than a method (we can use methods with nil receiver in Go, but the free function seems to be a better choice in this case). We introduce and use a new bytecounter specifically for the system resolver. This byte counter is very imprecise but seems still better than using the system resolver doesn't use any network I/O. We stop printing the sessionresolver stats when we close a session, since this component has been in production for years now. We improve the model.UnderlyingNetwork model to add support for changing the root cert pool, which helps with writing some integration tests. We modify the protocol to use and modify the netxlite tproxy (a singleton instance of UnderlyingNetwork) to make it goroutine safe. We introduce a new file inside sessionresolver, factory.go, which creates and properly wraps the resolvers. This code replaces code for which we previously used netx, and is the core change introduced here. While there, we refactor how we log in the session resolver to use the operation logger as we do in some experiments. We write some additional tests that take advantage of the new netxlite tproxy mocking functionality to ensure the sessionresolver continues to work in two extreme use cases: no resolver is available and just the system resolver is available. I introduced these new tests because I originally broke the system resolver when I introduced factory.go and I felt like it was useful to have more robustness here. --- internal/bytecounter/http.go | 6 +- internal/bytecounter/http_test.go | 4 +- internal/bytecounter/resolver.go | 110 ++++++ internal/bytecounter/resolver_test.go | 285 ++++++++++++++ internal/engine/session.go | 1 - internal/legacy/netx/http.go | 3 +- internal/model/mocks/underlyingnetwork.go | 8 + .../model/mocks/underlyingnetwork_test.go | 14 + internal/model/netx.go | 11 +- internal/netxlite/dialer.go | 2 +- internal/netxlite/dnsovergetaddrinfo.go | 4 +- internal/netxlite/quic.go | 2 +- internal/netxlite/tls.go | 2 +- internal/netxlite/tproxy.go | 38 +- internal/netxlite/tproxy_test.go | 19 + internal/sessionresolver/factory.go | 100 +++++ internal/sessionresolver/factory_test.go | 354 ++++++++++++++++++ internal/sessionresolver/resolver.go | 18 +- internal/sessionresolver/resolver_test.go | 146 +++++++- internal/sessionresolver/resolvermaker.go | 17 +- 20 files changed, 1101 insertions(+), 43 deletions(-) create mode 100644 internal/bytecounter/resolver.go create mode 100644 internal/bytecounter/resolver_test.go create mode 100644 internal/sessionresolver/factory.go create mode 100644 internal/sessionresolver/factory_test.go diff --git a/internal/bytecounter/http.go b/internal/bytecounter/http.go index f8926233b6..27f4feab79 100644 --- a/internal/bytecounter/http.go +++ b/internal/bytecounter/http.go @@ -10,9 +10,9 @@ import ( // MaybeWrapHTTPTransport takes in input an HTTPTransport and either wraps it // to perform byte counting, if this counter is not nil, or just returns to the // caller the original transport, when the counter is nil. -func (c *Counter) MaybeWrapHTTPTransport(txp model.HTTPTransport) model.HTTPTransport { - if c != nil { - txp = WrapHTTPTransport(txp, c) +func MaybeWrapHTTPTransport(txp model.HTTPTransport, counter *Counter) model.HTTPTransport { + if counter != nil { + txp = WrapHTTPTransport(txp, counter) } return txp } diff --git a/internal/bytecounter/http_test.go b/internal/bytecounter/http_test.go index 6616c015eb..7f3aa8b68a 100644 --- a/internal/bytecounter/http_test.go +++ b/internal/bytecounter/http_test.go @@ -16,7 +16,7 @@ func TestMaybeWrapHTTPTransport(t *testing.T) { t.Run("when counter is not nil", func(t *testing.T) { underlying := &mocks.HTTPTransport{} counter := &Counter{} - txp := counter.MaybeWrapHTTPTransport(underlying) + txp := MaybeWrapHTTPTransport(underlying, counter) realTxp := txp.(*httpTransport) if realTxp.HTTPTransport != underlying { t.Fatal("did not wrap correctly") @@ -26,7 +26,7 @@ func TestMaybeWrapHTTPTransport(t *testing.T) { t.Run("when counter is nil", func(t *testing.T) { underlying := &mocks.HTTPTransport{} var counter *Counter - txp := counter.MaybeWrapHTTPTransport(underlying) + txp := MaybeWrapHTTPTransport(underlying, counter) if txp != underlying { t.Fatal("unexpected result") } diff --git a/internal/bytecounter/resolver.go b/internal/bytecounter/resolver.go new file mode 100644 index 0000000000..c17d13202d --- /dev/null +++ b/internal/bytecounter/resolver.go @@ -0,0 +1,110 @@ +package bytecounter + +import ( + "context" + "net" + + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" +) + +// MaybeWrapSystemResolver takes in input an Resolver and either wraps it +// to perform byte counting, if this counter is not nil, or just returns to the +// caller the original transport, when the counter is nil. +// +// # Bug +// +// The returned resolver will only approximately estimate the bytes +// sent and received by this resolver if this resolver is the system +// resolver. For more accurate counting when using DNS over HTTPS, +// you should instead count at the HTTP transport level. If you are +// using DNS over TCP, DNS over TLS, or DNS over UDP, you should instead +// count the bytes by just wrapping the connections you're using. +func MaybeWrapSystemResolver(reso model.Resolver, counter *Counter) model.Resolver { + if counter != nil { + reso = WrapSystemResolver(reso, counter) + } + return reso +} + +// WrapSystemResolver creates a new byte-counting-aware resolver. This function +// returns a resolver with the same bugs of [MaybeWrapSystemResolver]. +func WrapSystemResolver(reso model.Resolver, counter *Counter) model.Resolver { + return &resolver{ + Resolver: reso, + Counter: counter, + } +} + +// resolver is the type returned by WrapResolver. +type resolver struct { + Resolver model.Resolver + Counter *Counter +} + +// Address implements model.Resolver +func (r *resolver) Address() string { + return r.Resolver.Address() +} + +// CloseIdleConnections implements model.Resolver +func (r *resolver) CloseIdleConnections() { + r.Resolver.CloseIdleConnections() +} + +// LookupHTTPS implements model.Resolver +func (r *resolver) LookupHTTPS(ctx context.Context, domain string) (*model.HTTPSSvc, error) { + r.updateCounterBytesSent(domain, 1) + out, err := r.Resolver.LookupHTTPS(ctx, domain) + r.updateCounterBytesRecv(err) + return out, err +} + +// LookupHost implements model.Resolver +func (r *resolver) LookupHost(ctx context.Context, hostname string) (addrs []string, err error) { + r.updateCounterBytesSent(hostname, 2) + out, err := r.Resolver.LookupHost(ctx, hostname) + r.updateCounterBytesRecv(err) + return out, err +} + +// LookupNS implements model.Resolver +func (r *resolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) { + r.updateCounterBytesSent(domain, 1) + out, err := r.Resolver.LookupNS(ctx, domain) + r.updateCounterBytesRecv(err) + return out, err +} + +// Network implements model.Resolver +func (r *resolver) Network() string { + return r.Resolver.Network() +} + +// updateCounterBytesSent estimates the bytes sent. +func (r *resolver) updateCounterBytesSent(domain string, n int) { + // Assume we are sending N queries for the given domain, which is the + // correct byte counting strategy when using the system resolver + r.Counter.Sent.Add(int64(n * len(domain))) +} + +// updateCounterBytesRecv estimates the bytes recv. +func (r *resolver) updateCounterBytesRecv(err error) { + if err != nil { + switch err.Error() { + case netxlite.FailureDNSNXDOMAINError, + netxlite.FailureDNSNoAnswer, + netxlite.FailureDNSRefusedError, + netxlite.FailureDNSNonRecoverableFailure, + netxlite.FailureDNSServfailError: + // In case it seems we received a message, let us + // pretent overall it was 128 bytes + r.Counter.Received.Add(128) + default: + // In this case we assume we did not receive any byte + } + return + } + // On success we assume we received 256 bytes + r.Counter.Received.Add(256) +} diff --git a/internal/bytecounter/resolver_test.go b/internal/bytecounter/resolver_test.go new file mode 100644 index 0000000000..4cda17295a --- /dev/null +++ b/internal/bytecounter/resolver_test.go @@ -0,0 +1,285 @@ +package bytecounter + +import ( + "context" + "errors" + "net" + "testing" + + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" + "github.com/ooni/probe-cli/v3/internal/netxlite" +) + +func TestMaybeWrapSystemResolver(t *testing.T) { + t.Run("we don't wrap when the counter is nil", func(t *testing.T) { + reso := &mocks.Resolver{} + out := MaybeWrapSystemResolver(reso, nil) + if reso != out { + t.Fatal("unexpected out") + } + }) + + t.Run("Address works as intended", func(t *testing.T) { + underlying := &mocks.Resolver{ + MockAddress: func() string { + return "8.8.8.8:53" + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + if reso.Address() != "8.8.8.8:53" { + t.Fatal("unexpected result") + } + }) + + t.Run("CloseIdleConnections works as intended", func(t *testing.T) { + var called bool + underlying := &mocks.Resolver{ + MockCloseIdleConnections: func() { + called = true + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + reso.CloseIdleConnections() + if !called { + t.Fatal("not called") + } + }) + + t.Run("LookupHTTPS works as intended", func(t *testing.T) { + t.Run("on success", func(t *testing.T) { + expected := &model.HTTPSSvc{} + underlying := &mocks.Resolver{ + MockLookupHTTPS: func(ctx context.Context, domain string) (*model.HTTPSSvc, error) { + return expected, nil + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupHTTPS(context.Background(), "dns.google") + if err != nil { + t.Fatal("unexpected error", err) + } + if got != expected { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 10 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 256 { + t.Fatal("unexpected nrecv") + } + }) + + t.Run("on non-DNS failure", func(t *testing.T) { + expected := errors.New("mocked error") + underlying := &mocks.Resolver{ + MockLookupHTTPS: func(ctx context.Context, domain string) (*model.HTTPSSvc, error) { + return nil, expected + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupHTTPS(context.Background(), "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + if got != nil { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 10 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 0 { + t.Fatal("unexpected nrecv") + } + }) + + t.Run("on DNS failure", func(t *testing.T) { + expected := errors.New(netxlite.FailureDNSNXDOMAINError) + underlying := &mocks.Resolver{ + MockLookupHTTPS: func(ctx context.Context, domain string) (*model.HTTPSSvc, error) { + return nil, expected + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupHTTPS(context.Background(), "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + if got != nil { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 10 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 128 { + t.Fatal("unexpected nrecv") + } + }) + }) + + t.Run("LookupNS works as intended", func(t *testing.T) { + t.Run("on success", func(t *testing.T) { + underlying := &mocks.Resolver{ + MockLookupNS: func(ctx context.Context, domain string) ([]*net.NS, error) { + out := make([]*net.NS, 3) + return out, nil + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupNS(context.Background(), "dns.google") + if err != nil { + t.Fatal("unexpected error", err) + } + if len(got) != 3 { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 10 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 256 { + t.Fatal("unexpected nrecv") + } + }) + + t.Run("on non-DNS failure", func(t *testing.T) { + expected := errors.New("mocked error") + underlying := &mocks.Resolver{ + MockLookupNS: func(ctx context.Context, domain string) ([]*net.NS, error) { + return nil, expected + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupNS(context.Background(), "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + if len(got) != 0 { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 10 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 0 { + t.Fatal("unexpected nrecv") + } + }) + + t.Run("on DNS failure", func(t *testing.T) { + expected := errors.New(netxlite.FailureDNSNXDOMAINError) + underlying := &mocks.Resolver{ + MockLookupNS: func(ctx context.Context, domain string) ([]*net.NS, error) { + return nil, expected + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupNS(context.Background(), "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + if len(got) != 0 { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 10 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 128 { + t.Fatal("unexpected nrecv") + } + }) + }) + + t.Run("LookupHost works as intended", func(t *testing.T) { + t.Run("on success", func(t *testing.T) { + underlying := &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + out := make([]string, 3) + return out, nil + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupHost(context.Background(), "dns.google") + if err != nil { + t.Fatal("unexpected error", err) + } + if len(got) != 3 { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 20 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 256 { + t.Fatal("unexpected nrecv") + } + }) + + t.Run("on non-DNS failure", func(t *testing.T) { + expected := errors.New("mocked error") + underlying := &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return nil, expected + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupHost(context.Background(), "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + if len(got) != 0 { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 20 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 0 { + t.Fatal("unexpected nrecv") + } + }) + + t.Run("on DNS failure", func(t *testing.T) { + expected := errors.New(netxlite.FailureDNSNXDOMAINError) + underlying := &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return nil, expected + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + got, err := reso.LookupHost(context.Background(), "dns.google") + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + if len(got) != 0 { + t.Fatal("invalid result") + } + if nsent := counter.BytesSent(); nsent != 20 { + t.Fatal("unexpected nsent", nsent) + } + if nrecv := counter.BytesReceived(); nrecv != 128 { + t.Fatal("unexpected nrecv") + } + }) + }) + + t.Run("Network works as intended", func(t *testing.T) { + underlying := &mocks.Resolver{ + MockNetwork: func() string { + return "udp" + }, + } + counter := New() + reso := MaybeWrapSystemResolver(underlying, counter) + if reso.Network() != "udp" { + t.Fatal("unexpected result") + } + }) +} diff --git a/internal/engine/session.go b/internal/engine/session.go index bec7efc956..3169d711ca 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -343,7 +343,6 @@ func (s *Session) Close() error { func (s *Session) doClose() { s.httpDefaultTransport.CloseIdleConnections() s.resolver.CloseIdleConnections() - s.logger.Infof("%s", s.resolver.Stats()) if s.tunnel != nil { s.tunnel.Stop() } diff --git a/internal/legacy/netx/http.go b/internal/legacy/netx/http.go index 9c5da196df..74314aa8b7 100644 --- a/internal/legacy/netx/http.go +++ b/internal/legacy/netx/http.go @@ -7,6 +7,7 @@ package netx import ( "crypto/tls" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -37,7 +38,7 @@ func NewHTTPTransport(config Config) model.HTTPTransport { // not super convinced by this code because it // seems we're currently counting bytes twice in some cases. I think we // should review how we're counting bytes and using netx currently. - txp = config.ByteCounter.MaybeWrapHTTPTransport(txp) // WAI with ByteCounter == nil + txp = bytecounter.MaybeWrapHTTPTransport(txp, config.ByteCounter) // WAI with ByteCounter == nil const defaultSnapshotSize = 0 // means: use the default snapsize return config.Saver.MaybeWrapHTTPTransport(txp, defaultSnapshotSize) // WAI with Saver == nil } diff --git a/internal/model/mocks/underlyingnetwork.go b/internal/model/mocks/underlyingnetwork.go index 830322acdf..3b3ccfdacd 100644 --- a/internal/model/mocks/underlyingnetwork.go +++ b/internal/model/mocks/underlyingnetwork.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "crypto/x509" "net" "time" @@ -17,6 +18,8 @@ type UnderlyingNetwork struct { MockGetaddrinfoLookupANY func(ctx context.Context, domain string) ([]string, string, error) MockGetaddrinfoResolverNetwork func() string + + MockMaybeModifyPool func(pool *x509.CertPool) *x509.CertPool } var _ model.UnderlyingNetwork = &UnderlyingNetwork{} @@ -36,3 +39,8 @@ func (un *UnderlyingNetwork) GetaddrinfoLookupANY(ctx context.Context, domain st func (un *UnderlyingNetwork) GetaddrinfoResolverNetwork() string { return un.MockGetaddrinfoResolverNetwork() } + +// MaybeModifyPool implements model.UnderlyingNetwork +func (un *UnderlyingNetwork) MaybeModifyPool(pool *x509.CertPool) *x509.CertPool { + return un.MockMaybeModifyPool(pool) +} diff --git a/internal/model/mocks/underlyingnetwork_test.go b/internal/model/mocks/underlyingnetwork_test.go index 9afc7a7186..d04c349a91 100644 --- a/internal/model/mocks/underlyingnetwork_test.go +++ b/internal/model/mocks/underlyingnetwork_test.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "crypto/x509" "errors" "net" "testing" @@ -76,4 +77,17 @@ func TestUnderlyingNetwork(t *testing.T) { t.Fatal("unexpected resolver network") } }) + + t.Run("MaybeModifyPool", func(t *testing.T) { + expect := x509.NewCertPool() + un := &UnderlyingNetwork{ + MockMaybeModifyPool: func(pool *x509.CertPool) *x509.CertPool { + return expect + }, + } + got := un.MaybeModifyPool(nil) + if got != expect { + t.Fatal("unexpected result") + } + }) } diff --git a/internal/model/netx.go b/internal/model/netx.go index a56c18b37f..26f8cdc396 100644 --- a/internal/model/netx.go +++ b/internal/model/netx.go @@ -7,6 +7,7 @@ package model import ( "context" "crypto/tls" + "crypto/x509" "net" "net/http" "syscall" @@ -488,13 +489,17 @@ type UnderlyingNetwork interface { // there is also an explicit timeout for dialing. DialContext(ctx context.Context, timeout time.Duration, network, address string) (net.Conn, error) - // ListenUDP is equivalent to net.ListenUDP. - ListenUDP(network string, addr *net.UDPAddr) (UDPLikeConn, error) - // GetaddrinfoLookupANY is like net.Resolver.LookupHost except that it // also returns to the caller the CNAME when it is available. GetaddrinfoLookupANY(ctx context.Context, domain string) ([]string, string, error) // GetaddrinfoResolverNetwork returns the resolver network. GetaddrinfoResolverNetwork() string + + // ListenUDP is equivalent to net.ListenUDP. + ListenUDP(network string, addr *net.UDPAddr) (UDPLikeConn, error) + + // MaybeModifyPool typically returns the same pool it is passed + // as an argument, but sometimes could modify it for testing. + MaybeModifyPool(pool *x509.CertPool) *x509.CertPool } diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index 79ab6c8adf..6b0b84e4bc 100644 --- a/internal/netxlite/dialer.go +++ b/internal/netxlite/dialer.go @@ -163,7 +163,7 @@ func (d *DialerSystem) configuredTimeout() time.Duration { } func (d *DialerSystem) DialContext(ctx context.Context, network, address string) (net.Conn, error) { - return TProxy.DialContext(ctx, d.configuredTimeout(), network, address) + return tproxySingleton().DialContext(ctx, d.configuredTimeout(), network, address) } func (d *DialerSystem) CloseIdleConnections() { diff --git a/internal/netxlite/dnsovergetaddrinfo.go b/internal/netxlite/dnsovergetaddrinfo.go index 34fec553e6..6ec3b7e166 100644 --- a/internal/netxlite/dnsovergetaddrinfo.go +++ b/internal/netxlite/dnsovergetaddrinfo.go @@ -104,7 +104,7 @@ func (txp *dnsOverGetaddrinfoTransport) lookupfn() func(ctx context.Context, dom if txp.testableLookupANY != nil { return txp.testableLookupANY } - return TProxy.GetaddrinfoLookupANY + return tproxySingleton().GetaddrinfoLookupANY } func (txp *dnsOverGetaddrinfoTransport) RequiresPadding() bool { @@ -112,7 +112,7 @@ func (txp *dnsOverGetaddrinfoTransport) RequiresPadding() bool { } func (txp *dnsOverGetaddrinfoTransport) Network() string { - return TProxy.GetaddrinfoResolverNetwork() + return tproxySingleton().GetaddrinfoResolverNetwork() } func (txp *dnsOverGetaddrinfoTransport) Address() string { diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index 682c976e04..16100862f5 100644 --- a/internal/netxlite/quic.go +++ b/internal/netxlite/quic.go @@ -29,7 +29,7 @@ var _ model.QUICListener = &quicListenerStdlib{} // Listen implements QUICListener.Listen. func (qls *quicListenerStdlib) Listen(addr *net.UDPAddr) (model.UDPLikeConn, error) { - return TProxy.ListenUDP("udp", addr) + return tproxySingleton().ListenUDP("udp", addr) } // NewQUICDialerWithResolver is the WrapDialer equivalent for QUIC where diff --git a/internal/netxlite/tls.go b/internal/netxlite/tls.go index dc87743a33..75367ede37 100644 --- a/internal/netxlite/tls.go +++ b/internal/netxlite/tls.go @@ -105,7 +105,7 @@ func NewDefaultCertPool() *x509.CertPool { // have a test in certify_test.go that guarantees that ok := pool.AppendCertsFromPEM([]byte(pemcerts)) runtimex.Assert(ok, "pool.AppendCertsFromPEM failed") - return pool + return tproxySingleton().MaybeModifyPool(pool) } // ErrInvalidTLSVersion indicates that you passed us a string diff --git a/internal/netxlite/tproxy.go b/internal/netxlite/tproxy.go index c56b853ec9..adc9c39ac5 100644 --- a/internal/netxlite/tproxy.go +++ b/internal/netxlite/tproxy.go @@ -2,19 +2,51 @@ package netxlite import ( "context" + "crypto/x509" "net" + "sync" "time" "github.com/ooni/probe-cli/v3/internal/model" ) -// TProxy refers to the UnderlyingNetwork implementation. By overriding this +// tproxySingletonInst refers to the UnderlyingNetwork implementation. By overriding this // variable you can force netxlite to use alternative network primitives. -var TProxy model.UnderlyingNetwork = &DefaultTProxy{} +var tproxySingletonInst model.UnderlyingNetwork = &DefaultTProxy{} -// defaultTProxy is the default UnderlyingNetwork implementation. +// tproxyMu protects the tproxySingleton. +var tproxyMu sync.Mutex + +// WithCustomTProxy runs the given function with a different UnderlyingNetwork +// and restores the previous UnderlyingNetwork before returning. +func WithCustomTProxy(tproxy model.UnderlyingNetwork, function func()) { + tproxyMu.Lock() + orig := tproxySingletonInst + tproxySingletonInst = tproxy + tproxyMu.Unlock() + defer func() { + tproxyMu.Lock() + tproxySingletonInst = orig + tproxyMu.Unlock() + }() + function() +} + +// tproxySingleton returns the tproxySingleton in a goroutine-safe way. +func tproxySingleton() model.UnderlyingNetwork { + defer tproxyMu.Unlock() + tproxyMu.Lock() + return tproxySingletonInst +} + +// DefaultTProxy is the default UnderlyingNetwork implementation. type DefaultTProxy struct{} +// MaybeModifyPool implements model.UnderlyingNetwork +func (tp *DefaultTProxy) MaybeModifyPool(pool *x509.CertPool) *x509.CertPool { + return pool +} + // DialContext implements UnderlyingNetwork. func (tp *DefaultTProxy) DialContext(ctx context.Context, timeout time.Duration, network, address string) (net.Conn, error) { d := &net.Dialer{ diff --git a/internal/netxlite/tproxy_test.go b/internal/netxlite/tproxy_test.go index c8c9cff674..e131e58998 100644 --- a/internal/netxlite/tproxy_test.go +++ b/internal/netxlite/tproxy_test.go @@ -2,10 +2,14 @@ package netxlite import ( "context" + "crypto/x509" "runtime" "strings" "testing" "time" + + "github.com/ooni/probe-cli/v3/internal/model/mocks" + "github.com/ooni/probe-cli/v3/internal/runtimex" ) func TestDefaultTProxy(t *testing.T) { @@ -30,3 +34,18 @@ func TestDefaultTProxy(t *testing.T) { } }) } + +func TestWithCustomTProxy(t *testing.T) { + expected := x509.NewCertPool() + tproxy := &mocks.UnderlyingNetwork{ + MockMaybeModifyPool: func(pool *x509.CertPool) *x509.CertPool { + runtimex.Assert(expected != pool, "got unexpected pool") + return expected + }, + } + WithCustomTProxy(tproxy, func() { + if NewDefaultCertPool() != expected { + t.Fatal("unexpected pool") + } + }) +} diff --git a/internal/sessionresolver/factory.go b/internal/sessionresolver/factory.go new file mode 100644 index 0000000000..397b786730 --- /dev/null +++ b/internal/sessionresolver/factory.go @@ -0,0 +1,100 @@ +package sessionresolver + +import ( + "errors" + "net/url" + + "github.com/ooni/probe-cli/v3/internal/bytecounter" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +// errCannotUseHTTP3WithAProxyURL means we cannot construct a new +// child resolver using HTTP/3 with a proxy URL. +var errCannotUseHTTP3WithAProxyURL = errors.New("cannot use HTTP/3 with a proxy URL") + +// errUnsupportedResolverScheme means we don't support the +// given resolver scheme. We only support https, http and system. +var errUnsupportedResolverScheme = errors.New("unsupported resolver scheme") + +// newChildResolver constructs a new child resolver. +// +// Arguments: +// +// - logger is the MANDATORY logger; +// +// - URL is the MANDATORY HTTP/3 URL to use; +// +// - http3Enabled indicates whether to use HTTP/3; +// +// - counter is the OPTIONAL byte counter; +// +// - proxyURL is the OPTIONAL proxy URL. +// +// Using a proxy URL is incompatible with using HTTP/3 and this +// factory will return an error if that happens. +// +// This function returns a model.Resolver or an error. +func newChildResolver( + logger model.Logger, + URL string, + http3Enabled bool, + counter *bytecounter.Counter, + proxyURL *url.URL, +) (model.Resolver, error) { + runtimex.Assert(logger != nil, "passed a nil model.Logger") + runtimex.Assert(URL != "", "passed an empty URL") + if http3Enabled && proxyURL != nil { + return nil, errCannotUseHTTP3WithAProxyURL + } + parsed, err := url.Parse(URL) + if err != nil { + return nil, err + } + var reso model.Resolver + switch parsed.Scheme { + case "http", "https": + reso = newChildResolverHTTPS(logger, URL, http3Enabled, counter, proxyURL) + case "system": + reso = bytecounter.MaybeWrapSystemResolver( + netxlite.NewStdlibResolver(logger), + counter, // handles correctly the case where counter is nil + ) + default: + return nil, errUnsupportedResolverScheme + } + reso = netxlite.MaybeWrapWithBogonResolver(true, reso) + return reso, nil +} + +// newChildResolverHTTPS is like newChildResolver but assumes that +// we already know that the URL scheme is http or https. +func newChildResolverHTTPS( + logger model.Logger, + URL string, + http3Enabled bool, + counter *bytecounter.Counter, + proxyURL *url.URL, +) model.Resolver { + var txp model.HTTPTransport + switch http3Enabled { + case false: + dialer := netxlite.MaybeWrapWithProxyDialer( + netxlite.NewDialerWithStdlibResolver(logger), + proxyURL, // handles correctly the case where proxyURL is nil + ) + thx := netxlite.NewTLSHandshakerStdlib(logger) + tlsDialer := netxlite.NewTLSDialer(dialer, thx) + txp = netxlite.NewHTTPTransport(logger, dialer, tlsDialer) + case true: + // TODO(bassosimone): to test this arm we need to further extend + // netxlite to override the default list of certificates + txp = netxlite.NewHTTP3TransportStdlib(logger) + } + txp = bytecounter.MaybeWrapHTTPTransport(txp, counter) + dnstxp := netxlite.NewDNSOverHTTPSTransportWithHTTPTransport(txp, URL) + underlying := netxlite.NewUnwrappedParallelResolver(dnstxp) + wrapped := netxlite.WrapResolver(logger, underlying) + return wrapped +} diff --git a/internal/sessionresolver/factory_test.go b/internal/sessionresolver/factory_test.go new file mode 100644 index 0000000000..024f2e5bdd --- /dev/null +++ b/internal/sessionresolver/factory_test.go @@ -0,0 +1,354 @@ +package sessionresolver + +import ( + "context" + "errors" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "syscall" + "testing" + + "github.com/miekg/dns" + "github.com/ooni/probe-cli/v3/internal/bytecounter" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/netxlite/filtering" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +// testDNSOverHTTPSHandler is an [http.Handler] serving DNS over HTTPS. +type testDNSOverHTTPSHandler struct { + // A contains the addresses to return. + A []net.IP +} + +var _ http.Handler = &testDNSOverHTTPSHandler{} + +// ServeHTTP implements http.Handler +func (h *testDNSOverHTTPSHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + rawQuery, err := netxlite.ReadAllContext(r.Context(), r.Body) + if err != nil { + panic(err) + } + query := &dns.Msg{} + if err := query.Unpack(rawQuery); err != nil { + panic(err) + } + runtimex.Assert(len(query.Question) == 1, "expected a single question") + resp := &dns.Msg{} + resp.SetReply(query) + resp.Compress = true + resp.RecursionAvailable = true + question0 := query.Question[0] + switch question0.Qtype { + case dns.TypeA: + for _, entry := range h.A { + resp.Answer = append(resp.Answer, &dns.A{ + Hdr: dns.RR_Header{ + Name: question0.Name, + Rrtype: dns.TypeA, + Class: dns.ClassINET, + Ttl: 0, + }, + A: entry, + }) + } + default: + // nothing + } + rawResp, err := resp.Pack() + if err != nil { + panic(err) + } + w.Header().Add("content-type", "application/dns-message") + w.Write(rawResp) + +} + +func Test_newChildResolver(t *testing.T) { + t.Run("we cannot create an HTTP3 enabled resolver with a proxy URL", func(t *testing.T) { + reso, err := newChildResolver( + model.DiscardLogger, + "https://www.google.com", + true, + bytecounter.New(), + &url.URL{}, // even an empty URL is enough + ) + if !errors.Is(err, errCannotUseHTTP3WithAProxyURL) { + t.Fatal("unexpected error", err) + } + if reso != nil { + t.Fatal("expected nil resolver here") + } + }) + + t.Run("we return an error when we cannot parse the resolver URL", func(t *testing.T) { + reso, err := newChildResolver( + model.DiscardLogger, + "\t", + true, + bytecounter.New(), + nil, + ) + if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { + t.Fatal("unexpected error", err) + } + if reso != nil { + t.Fatal("expected nil resolver here") + } + }) + + t.Run("we return an error when we don't support the URL scheme", func(t *testing.T) { + reso, err := newChildResolver( + model.DiscardLogger, + "dot://8.8.8.8:853/", + true, + bytecounter.New(), + nil, + ) + if !errors.Is(err, errUnsupportedResolverScheme) { + t.Fatal("unexpected error", err) + } + if reso != nil { + t.Fatal("expected nil resolver here") + } + }) + + t.Run("for HTTPS resolvers", func(t *testing.T) { + + t.Run("the returned resolver wraps errors", func(t *testing.T) { + srvr := filtering.NewHTTPServerCleartext(filtering.HTTPActionReset) + defer srvr.Close() + reso, err := newChildResolver( + model.DiscardLogger, + srvr.URL().String(), + false, + bytecounter.New(), + nil, + ) + if err != nil { + t.Fatal(err) + } + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err == nil || err.Error() != netxlite.FailureConnectionReset { + t.Fatal("unexpected error", err) + } + if len(addrs) != 0 { + t.Fatal("expected zero length addrs here") + } + }) + + t.Run("what we get is an action DNS-over-HTTPS resolver", func(t *testing.T) { + handler := &testDNSOverHTTPSHandler{ + A: []net.IP{net.IPv4(8, 8, 8, 8)}, + } + srvr := httptest.NewServer(handler) + defer srvr.Close() + + reso, err := newChildResolver( + model.DiscardLogger, + srvr.URL, + false, + bytecounter.New(), + nil, + ) + if err != nil { + t.Fatal(err) + } + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err != nil { + t.Fatal("unexpected error", err) + } + + if len(addrs) != 1 { + t.Fatal("expected a single addr here") + } + if addrs[0] != "8.8.8.8" { + t.Fatal("unexpected addr") + } + }) + + t.Run("we count the bytes received and sent", func(t *testing.T) { + counter := bytecounter.New() + + handler := &testDNSOverHTTPSHandler{ + A: []net.IP{net.IPv4(8, 8, 8, 8)}, + } + srvr := httptest.NewServer(handler) + defer srvr.Close() + + reso, err := newChildResolver( + model.DiscardLogger, + srvr.URL, + false, + counter, + nil, + ) + if err != nil { + t.Fatal(err) + } + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err != nil { + t.Fatal("unexpected error", err) + } + + if len(addrs) != 1 { + t.Fatal("expected a single addr here") + } + if addrs[0] != "8.8.8.8" { + t.Fatal("unexpected addr") + } + + if counter.BytesReceived() <= 0 { + t.Fatal("expected to see received bytes") + } + + if counter.BytesSent() <= 0 { + t.Fatal("expected to see sent bytes") + } + }) + + t.Run("the returned resolver is such that we reject bogons", func(t *testing.T) { + + handler := &testDNSOverHTTPSHandler{ + A: []net.IP{net.IPv4(10, 10, 34, 34)}, + } + srvr := httptest.NewServer(handler) + defer srvr.Close() + + reso, err := newChildResolver( + model.DiscardLogger, + srvr.URL, + false, + bytecounter.New(), + nil, + ) + if err != nil { + t.Fatal(err) + } + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err == nil || err.Error() != netxlite.FailureDNSBogonError { + t.Fatal("unexpected error", err) + } + + if len(addrs) != 0 { + t.Fatal("expected no addrs here") + } + }) + }) + + t.Run("for the system resolver", func(t *testing.T) { + + t.Run("the returned resolver wraps errors", func(t *testing.T) { + tproxy := &mocks.UnderlyingNetwork{ + MockGetaddrinfoLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + return nil, "", syscall.ENETDOWN + }, + MockGetaddrinfoResolverNetwork: func() string { + return netxlite.StdlibResolverGetaddrinfo + }, + } + netxlite.WithCustomTProxy(tproxy, func() { + reso, err := newChildResolver( + model.DiscardLogger, + "system:///", + false, + bytecounter.New(), + nil, + ) + if err != nil { + t.Fatal(err) + } + + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err == nil || err.Error() != netxlite.FailureNetworkDown { + t.Fatal("unexpected error", err) + } + if len(addrs) != 0 { + t.Fatal("expected zero length addrs here") + } + }) + }) + + t.Run("the returned resolver is such that we reject bogons", func(t *testing.T) { + tproxy := &mocks.UnderlyingNetwork{ + MockGetaddrinfoLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + addrs := []string{"10.10.34.34"} + return addrs, "", nil + }, + MockGetaddrinfoResolverNetwork: func() string { + return netxlite.StdlibResolverGetaddrinfo + }, + } + netxlite.WithCustomTProxy(tproxy, func() { + reso, err := newChildResolver( + model.DiscardLogger, + "system:///", + false, + bytecounter.New(), + nil, + ) + if err != nil { + t.Fatal(err) + } + + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err == nil || err.Error() != netxlite.FailureDNSBogonError { + t.Fatal("unexpected error", err) + } + if len(addrs) != 0 { + t.Fatal("expected zero length addrs here") + } + }) + }) + + t.Run("we count the bytes sent and received", func(t *testing.T) { + counter := bytecounter.New() + + tproxy := &mocks.UnderlyingNetwork{ + MockGetaddrinfoLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + addrs := []string{"8.8.8.8"} + return addrs, "", nil + }, + MockGetaddrinfoResolverNetwork: func() string { + return netxlite.StdlibResolverGetaddrinfo + }, + } + netxlite.WithCustomTProxy(tproxy, func() { + reso, err := newChildResolver( + model.DiscardLogger, + "system:///", + false, + counter, + nil, + ) + if err != nil { + t.Fatal(err) + } + + addrs, err := reso.LookupHost(context.Background(), "dns.google") + if err != nil { + t.Fatal("unexpected error", err) + } + if len(addrs) != 1 { + t.Fatal("expected a single addr here") + } + if addrs[0] != "8.8.8.8" { + t.Fatal("unexpected addr") + } + + if counter.BytesReceived() <= 0 { + t.Fatal("expected to see received bytes") + } + + if counter.BytesSent() <= 0 { + t.Fatal("expected to see sent bytes") + } + }) + }) + }) +} diff --git a/internal/sessionresolver/resolver.go b/internal/sessionresolver/resolver.go index a8b96bc996..0b0b124ae9 100644 --- a/internal/sessionresolver/resolver.go +++ b/internal/sessionresolver/resolver.go @@ -1,14 +1,12 @@ package sessionresolver // -// Implementation of Resolver +// Implementation of model.Resolver // import ( "context" - "encoding/json" "errors" - "fmt" "math/rand" "net" "net/url" @@ -16,9 +14,9 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/bytecounter" + "github.com/ooni/probe-cli/v3/internal/measurexlite" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/multierror" - "github.com/ooni/probe-cli/v3/internal/runtimex" ) // Resolver is the session resolver. Resolver will try to use @@ -82,13 +80,6 @@ func (r *Resolver) CloseIdleConnections() { r.once.Do(r.closeall) } -// Stats returns stats about the session resolver. -func (r *Resolver) Stats() string { - data, err := json.Marshal(r.readstatedefault()) - runtimex.PanicOnError(err, "json.Marshal should not fail here") - return fmt.Sprintf("sessionresolver: %s", string(data)) -} - // errLookupNotImplemented indicates a given lookup type is not implemented. var errLookupNotImplemented = errors.New("sessionresolver: lookup not implemented") @@ -148,13 +139,14 @@ func (r *Resolver) lookupHost(ctx context.Context, ri *resolverinfo, hostname st ri.Score = 0 // this is a hard error return nil, err } + op := measurexlite.NewOperationLogger( + r.logger(), "sessionresolver: lookup %s using %s", hostname, ri.URL) addrs, err := timeLimitedLookup(ctx, re, hostname) + op.Stop(err) if err == nil { - r.logger().Infof("sessionresolver: %s... %v", ri.URL, model.ErrorToStringOrOK(nil)) ri.Score = ewma*1.0 + (1-ewma)*ri.Score // increase score return addrs, nil } - r.logger().Warnf("sessionresolver: %s... %s", ri.URL, err.Error()) ri.Score = ewma*0.0 + (1-ewma)*ri.Score // decrease score return nil, err } diff --git a/internal/sessionresolver/resolver_test.go b/internal/sessionresolver/resolver_test.go index fa15b38a12..dd64e575a3 100644 --- a/internal/sessionresolver/resolver_test.go +++ b/internal/sessionresolver/resolver_test.go @@ -2,18 +2,23 @@ package sessionresolver import ( "context" + "crypto/x509" "errors" "net" "net/url" "strings" "sync/atomic" + "syscall" "testing" + "time" "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/model/mocks" "github.com/ooni/probe-cli/v3/internal/multierror" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestNetworkWorks(t *testing.T) { @@ -72,9 +77,6 @@ func TestTypicalUsageWithFailure(t *testing.T) { if len(reso.res) < 1 { t.Fatal("expected to see some resolvers here") } - if reso.Stats() == "" { - t.Fatal("expected to see some string returned by stats") - } reso.CloseIdleConnections() if len(reso.res) != 0 { t.Fatal("expected to see no resolvers after CloseIdleConnections") @@ -384,3 +386,141 @@ func TestUnimplementedFunctions(t *testing.T) { } }) } + +func TestResolverWorkingAsIntendedWithMocks(t *testing.T) { + + // fields contains the public fields to set. + type fields struct { + // byteCounter is the byte counter we'll use. + byteCounter *bytecounter.Counter + + // kvstore is the kvstore we'll use. + kvstore *kvstore.Memory + + // logger is the logger we'll use. + logger model.Logger + + // proxyURL is the proxy URL we'll use. + proxyURL *url.URL + } + + // testCase is an individual test case. + type testCase struct { + // name is the test case name. + name string + + // fields contains the fields to set. + fields *fields + + // domainToResolve is the domain to resolve. + domainToResolve string + + // tproxy contains the netxlite underlying network + // configuration to use for testing. + tproxy model.UnderlyingNetwork + + // expectErr indicates whether we expected an error. + expectErr bool + + // expectAddrs contains the expected addresses. + expectAddrs []string + } + + // TODO(bassosimone): as painful as it may be, we need to write more + // tests like this that capture the whole behavior of the package. They + // give us higher confidence that _everything_ is still WAI regardless + // of any intermediate refactoring we may be implement here. + // + // For now, I have just written tests for extreme use cases such as + // nothing is working and just the system resolver is working. We need + // to figure out a way of writing more alike tests. + // + // I am not going to do that now, because that would be out of the + // scope of the current pull request on which I am working. + + var testCases = []testCase{{ + name: "every system-resolver lookup returns NXDOMAIN", + fields: &fields{ + byteCounter: bytecounter.New(), + kvstore: &kvstore.Memory{}, + logger: model.DiscardLogger, + proxyURL: nil, + }, + domainToResolve: "example.com", + tproxy: &mocks.UnderlyingNetwork{ + MockDialContext: func(ctx context.Context, timeout time.Duration, network string, address string) (net.Conn, error) { + dialer := &net.Dialer{Timeout: timeout} + return dialer.DialContext(ctx, network, address) + }, + MockListenUDP: func(network string, addr *net.UDPAddr) (model.UDPLikeConn, error) { + return net.ListenUDP(network, addr) + }, + MockGetaddrinfoLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + return nil, "", errors.New(netxlite.DNSNoSuchHostSuffix) + }, + MockGetaddrinfoResolverNetwork: func() string { + return netxlite.StdlibResolverGetaddrinfo + }, + MockMaybeModifyPool: func(pool *x509.CertPool) *x509.CertPool { + return pool + }, + }, + expectErr: true, + expectAddrs: nil, + }, { + name: "only the system resolver works", + fields: &fields{ + byteCounter: bytecounter.New(), + kvstore: &kvstore.Memory{}, + logger: model.DiscardLogger, + proxyURL: nil, + }, + domainToResolve: "example.com", + tproxy: &mocks.UnderlyingNetwork{ + MockDialContext: func(ctx context.Context, timeout time.Duration, network string, address string) (net.Conn, error) { + return nil, syscall.ECONNREFUSED + }, + MockListenUDP: func(network string, addr *net.UDPAddr) (model.UDPLikeConn, error) { + return nil, syscall.ENETDOWN + }, + MockGetaddrinfoLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { + switch domain { + case "example.com": + return []string{"1.2.3.4", "1.2.3.5"}, "", nil + default: + return nil, "", errors.New(netxlite.DNSNoSuchHostSuffix) + } + }, + MockGetaddrinfoResolverNetwork: func() string { + return netxlite.StdlibResolverGetaddrinfo + }, + MockMaybeModifyPool: func(pool *x509.CertPool) *x509.CertPool { + return pool + }, + }, + expectErr: false, + expectAddrs: []string{"1.2.3.4", "1.2.3.5"}, + }} + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + netxlite.WithCustomTProxy(tt.tproxy, func() { + reso := &Resolver{ + ByteCounter: tt.fields.byteCounter, + KVStore: tt.fields.kvstore, + Logger: tt.fields.logger, + ProxyURL: tt.fields.proxyURL, + } + + addrs, err := reso.LookupHost(context.Background(), tt.domainToResolve) + if (err != nil) != tt.expectErr { + t.Fatal("tt.expectErr", tt.expectErr, "got", err) + } + + if diff := cmp.Diff(tt.expectAddrs, addrs); diff != "" { + t.Fatal(diff) + } + }) + }) + } +} diff --git a/internal/sessionresolver/resolvermaker.go b/internal/sessionresolver/resolvermaker.go index 2bd7f79fd2..40359f6e42 100644 --- a/internal/sessionresolver/resolvermaker.go +++ b/internal/sessionresolver/resolvermaker.go @@ -1,7 +1,7 @@ package sessionresolver // -// Code for creating a new child resolver +// High-level for creating a new child resolver // import ( @@ -9,7 +9,6 @@ import ( "strings" "time" - "github.com/ooni/probe-cli/v3/internal/legacy/netx" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -71,13 +70,13 @@ func (r *Resolver) newChildResolver(h3 bool, URL string) (model.Resolver, error) if r.newChildResolverFn != nil { return r.newChildResolverFn(h3, URL) } - return netx.NewDNSClient(netx.Config{ - BogonIsError: true, - ByteCounter: r.ByteCounter, // nil is handled by netx - HTTP3Enabled: h3, - Logger: r.logger(), - ProxyURL: r.ProxyURL, - }, URL) + return newChildResolver( + r.logger(), + URL, + h3, + r.ByteCounter, // newChildResolver handles the nil case + r.ProxyURL, // ditto + ) } // newresolver creates a new resolver with the given config and URL. This is From ba10aad5e61aab3483fcf38531b5730fad5d7561 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:08:39 +0100 Subject: [PATCH 2/9] [ci skip] Update internal/bytecounter/resolver.go --- internal/bytecounter/resolver.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/bytecounter/resolver.go b/internal/bytecounter/resolver.go index c17d13202d..f03cbd916f 100644 --- a/internal/bytecounter/resolver.go +++ b/internal/bytecounter/resolver.go @@ -8,9 +8,9 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite" ) -// MaybeWrapSystemResolver takes in input an Resolver and either wraps it +// MaybeWrapSystemResolver takes in input a Resolver and either wraps it // to perform byte counting, if this counter is not nil, or just returns to the -// caller the original transport, when the counter is nil. +// caller the original resolver, when the counter is nil. // // # Bug // From d8f504b990f5a4cf271d05083406b2aa38e574f7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:08:49 +0100 Subject: [PATCH 3/9] [ci skip] Update internal/bytecounter/resolver.go --- internal/bytecounter/resolver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/bytecounter/resolver.go b/internal/bytecounter/resolver.go index f03cbd916f..1a86153b73 100644 --- a/internal/bytecounter/resolver.go +++ b/internal/bytecounter/resolver.go @@ -98,7 +98,7 @@ func (r *resolver) updateCounterBytesRecv(err error) { netxlite.FailureDNSNonRecoverableFailure, netxlite.FailureDNSServfailError: // In case it seems we received a message, let us - // pretent overall it was 128 bytes + // pretend overall it was 128 bytes r.Counter.Received.Add(128) default: // In this case we assume we did not receive any byte From 9a76899e8a91e75d0a325f5d3fddf9f885994233 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:08:59 +0100 Subject: [PATCH 4/9] [ci skip] Update internal/sessionresolver/resolvermaker.go --- internal/sessionresolver/resolvermaker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sessionresolver/resolvermaker.go b/internal/sessionresolver/resolvermaker.go index 40359f6e42..db337fec95 100644 --- a/internal/sessionresolver/resolvermaker.go +++ b/internal/sessionresolver/resolvermaker.go @@ -1,7 +1,7 @@ package sessionresolver // -// High-level for creating a new child resolver +// High-level code for creating a new child resolver // import ( From fd2fbe96dd1ea3e3fbf073d0e0913f2139ec9a1a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:09:09 +0100 Subject: [PATCH 5/9] [ci skip] Update internal/sessionresolver/factory.go --- internal/sessionresolver/factory.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sessionresolver/factory.go b/internal/sessionresolver/factory.go index 397b786730..bd2c05ab64 100644 --- a/internal/sessionresolver/factory.go +++ b/internal/sessionresolver/factory.go @@ -24,7 +24,7 @@ var errUnsupportedResolverScheme = errors.New("unsupported resolver scheme") // // - logger is the MANDATORY logger; // -// - URL is the MANDATORY HTTP/3 URL to use; +// - URL is the MANDATORY URL to use (a DoH URL or system:///); // // - http3Enabled indicates whether to use HTTP/3; // From 9c286530ef9980db3d3a9b4f5b5bfd71f0833bc8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:09:18 +0100 Subject: [PATCH 6/9] [ci skip] Update internal/sessionresolver/factory.go --- internal/sessionresolver/factory.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sessionresolver/factory.go b/internal/sessionresolver/factory.go index bd2c05ab64..3491c7ab7b 100644 --- a/internal/sessionresolver/factory.go +++ b/internal/sessionresolver/factory.go @@ -54,7 +54,7 @@ func newChildResolver( } var reso model.Resolver switch parsed.Scheme { - case "http", "https": + case "http", "https": // http is here for testing reso = newChildResolverHTTPS(logger, URL, http3Enabled, counter, proxyURL) case "system": reso = bytecounter.MaybeWrapSystemResolver( From fc26921743688d474ff3c1f14597619897e4361d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:09:29 +0100 Subject: [PATCH 7/9] [ci skip] Update internal/sessionresolver/factory.go --- internal/sessionresolver/factory.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/sessionresolver/factory.go b/internal/sessionresolver/factory.go index 3491c7ab7b..2b7cc3c12c 100644 --- a/internal/sessionresolver/factory.go +++ b/internal/sessionresolver/factory.go @@ -88,8 +88,6 @@ func newChildResolverHTTPS( tlsDialer := netxlite.NewTLSDialer(dialer, thx) txp = netxlite.NewHTTPTransport(logger, dialer, tlsDialer) case true: - // TODO(bassosimone): to test this arm we need to further extend - // netxlite to override the default list of certificates txp = netxlite.NewHTTP3TransportStdlib(logger) } txp = bytecounter.MaybeWrapHTTPTransport(txp, counter) From 3f026e53c7df62da0033c5f28304d374f87c769a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 18:09:38 +0100 Subject: [PATCH 8/9] [ci skip] Update internal/sessionresolver/factory_test.go --- internal/sessionresolver/factory_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sessionresolver/factory_test.go b/internal/sessionresolver/factory_test.go index 024f2e5bdd..16d1e037d0 100644 --- a/internal/sessionresolver/factory_test.go +++ b/internal/sessionresolver/factory_test.go @@ -142,7 +142,7 @@ func Test_newChildResolver(t *testing.T) { } }) - t.Run("what we get is an action DNS-over-HTTPS resolver", func(t *testing.T) { + t.Run("what we get is a DNS-over-HTTPS resolver", func(t *testing.T) { handler := &testDNSOverHTTPSHandler{ A: []net.IP{net.IPv4(8, 8, 8, 8)}, } From 3ae3ad417c344009cc72f0f6018e423b4989bd67 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Feb 2023 17:17:13 +0000 Subject: [PATCH 9/9] x --- internal/sessionresolver/factory_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/sessionresolver/factory_test.go b/internal/sessionresolver/factory_test.go index 16d1e037d0..73fa91ff9c 100644 --- a/internal/sessionresolver/factory_test.go +++ b/internal/sessionresolver/factory_test.go @@ -16,7 +16,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/model/mocks" "github.com/ooni/probe-cli/v3/internal/netxlite" - "github.com/ooni/probe-cli/v3/internal/netxlite/filtering" "github.com/ooni/probe-cli/v3/internal/runtimex" ) @@ -121,11 +120,18 @@ func Test_newChildResolver(t *testing.T) { t.Run("for HTTPS resolvers", func(t *testing.T) { t.Run("the returned resolver wraps errors", func(t *testing.T) { - srvr := filtering.NewHTTPServerCleartext(filtering.HTTPActionReset) + handler := &testDNSOverHTTPSHandler{ + A: []net.IP{net.IPv4(8, 8, 8, 8)}, + } + // Because we're using a testing server w/o installing its + // certificate, we expect to see a TLS failure here + srvr := httptest.NewTLSServer(handler) + defer srvr.Close() + defer srvr.Close() reso, err := newChildResolver( model.DiscardLogger, - srvr.URL().String(), + srvr.URL, false, bytecounter.New(), nil, @@ -134,7 +140,7 @@ func Test_newChildResolver(t *testing.T) { t.Fatal(err) } addrs, err := reso.LookupHost(context.Background(), "dns.google") - if err == nil || err.Error() != netxlite.FailureConnectionReset { + if err == nil || err.Error() != netxlite.FailureSSLUnknownAuthority { t.Fatal("unexpected error", err) } if len(addrs) != 0 {