From f1f5ed342e29212079540c6ad1340aef9880fe57 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 25 Jun 2021 18:38:13 +0200 Subject: [PATCH] refactor: move quic dns dialing to netxlite (#408) Part of https://github.com/ooni/probe/issues/1505 --- internal/engine/netx/netx.go | 2 +- internal/engine/netx/quicdialer/dns.go | 57 -------- internal/engine/netx/quicdialer/dns_test.go | 149 -------------------- internal/netxlite/dialer.go | 2 +- internal/netxlite/legacy.go | 4 +- internal/netxlite/legacy_test.go | 8 +- internal/netxlite/quic.go | 61 ++++++-- internal/netxlite/quic_test.go | 92 ++++++++++++ 8 files changed, 152 insertions(+), 223 deletions(-) delete mode 100644 internal/engine/netx/quicdialer/dns.go delete mode 100644 internal/engine/netx/quicdialer/dns_test.go diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 111359d..eb25850 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -174,7 +174,7 @@ func NewQUICDialer(config Config) QUICDialer { if config.TLSSaver != nil { d = quicdialer.HandshakeSaver{Saver: config.TLSSaver, Dialer: d} } - d = &quicdialer.DNSDialer{Resolver: config.FullResolver, Dialer: d} + d = &netxlite.QUICDialerResolver{Resolver: config.FullResolver, Dialer: d} var dialer QUICDialer = &httptransport.QUICWrapperDialer{Dialer: d} return dialer } diff --git a/internal/engine/netx/quicdialer/dns.go b/internal/engine/netx/quicdialer/dns.go deleted file mode 100644 index 9d83be6..0000000 --- a/internal/engine/netx/quicdialer/dns.go +++ /dev/null @@ -1,57 +0,0 @@ -package quicdialer - -import ( - "context" - "crypto/tls" - "net" - - "github.com/lucas-clemente/quic-go" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -// DNSDialer is a dialer that uses the configured Resolver to resolve a -// domain name to IP addresses -type DNSDialer struct { - Dialer ContextDialer - Resolver Resolver -} - -// DialContext implements ContextDialer.DialContext -func (d DNSDialer) DialContext( - ctx context.Context, network, host string, - tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlySession, error) { - onlyhost, onlyport, err := net.SplitHostPort(host) - if err != nil { - return nil, err - } - // TODO(kelmenhorst): Should this be somewhere else? - // failure if tlsCfg is nil but that should not happen - if tlsCfg.ServerName == "" { - tlsCfg.ServerName = onlyhost - } - var addrs []string - addrs, err = d.LookupHost(ctx, onlyhost) - if err != nil { - return nil, err - } - var errorslist []error - for _, addr := range addrs { - target := net.JoinHostPort(addr, onlyport) - sess, err := d.Dialer.DialContext( - ctx, network, target, tlsCfg, cfg) - if err == nil { - return sess, nil - } - errorslist = append(errorslist, err) - } - // TODO(bassosimone): maybe ReduceErrors could be in netx/internal. - return nil, netxlite.ReduceErrors(errorslist) -} - -// LookupHost implements Resolver.LookupHost -func (d DNSDialer) LookupHost(ctx context.Context, hostname string) ([]string, error) { - if net.ParseIP(hostname) != nil { - return []string{hostname}, nil - } - return d.Resolver.LookupHost(ctx, hostname) -} diff --git a/internal/engine/netx/quicdialer/dns_test.go b/internal/engine/netx/quicdialer/dns_test.go deleted file mode 100644 index 9d485fb..0000000 --- a/internal/engine/netx/quicdialer/dns_test.go +++ /dev/null @@ -1,149 +0,0 @@ -package quicdialer_test - -import ( - "context" - "crypto/tls" - "errors" - "net" - "strconv" - "strings" - "testing" - - "github.com/lucas-clemente/quic-go" - "github.com/ooni/probe-cli/v3/internal/engine/netx/quicdialer" - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -type MockableResolver struct { - Addresses []string - Err error -} - -func (r MockableResolver) LookupHost(ctx context.Context, host string) ([]string, error) { - return r.Addresses, r.Err -} - -func TestDNSDialerSuccess(t *testing.T) { - tlsConf := &tls.Config{NextProtos: []string{"h3"}} - dialer := quicdialer.DNSDialer{ - Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{ - QUICListener: &netxlite.QUICListenerStdlib{}, - }} - sess, err := dialer.DialContext( - context.Background(), "udp", "www.google.com:443", - tlsConf, &quic.Config{}) - if err != nil { - t.Fatal("unexpected error", err) - } - if sess == nil { - t.Fatal("non nil sess expected") - } -} - -func TestDNSDialerNoPort(t *testing.T) { - tlsConf := &tls.Config{NextProtos: []string{"h3"}} - dialer := quicdialer.DNSDialer{ - Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{}} - sess, err := dialer.DialContext( - context.Background(), "udp", "www.google.com", - tlsConf, &quic.Config{}) - if err == nil { - t.Fatal("expected an error here") - } - if sess != nil { - t.Fatal("expected a nil sess here") - } - if err.Error() != "address www.google.com: missing port in address" { - t.Fatal("not the error we expected") - } -} - -func TestDNSDialerLookupHostAddress(t *testing.T) { - dialer := quicdialer.DNSDialer{Resolver: MockableResolver{ - Err: errors.New("mocked error"), - }} - addrs, err := dialer.LookupHost(context.Background(), "1.1.1.1") - if err != nil { - t.Fatal(err) - } - if len(addrs) != 1 || addrs[0] != "1.1.1.1" { - t.Fatal("not the result we expected") - } -} - -func TestDNSDialerLookupHostFailure(t *testing.T) { - tlsConf := &tls.Config{NextProtos: []string{"h3"}} - expected := errors.New("mocked error") - dialer := quicdialer.DNSDialer{Resolver: MockableResolver{ - Err: expected, - }} - sess, err := dialer.DialContext( - context.Background(), "udp", "dns.google.com:853", - tlsConf, &quic.Config{}) - if !errors.Is(err, expected) { - t.Fatal("not the error we expected") - } - if sess != nil { - t.Fatal("expected nil sess") - } -} - -func TestDNSDialerInvalidPort(t *testing.T) { - tlsConf := &tls.Config{NextProtos: []string{"h3"}} - dialer := quicdialer.DNSDialer{ - Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{ - QUICListener: &netxlite.QUICListenerStdlib{}, - }} - sess, err := dialer.DialContext( - context.Background(), "udp", "www.google.com:0", - tlsConf, &quic.Config{}) - if err == nil { - t.Fatal("expected an error here") - } - if sess != nil { - t.Fatal("expected nil sess") - } - if !strings.HasSuffix(err.Error(), "sendto: invalid argument") && - !strings.HasSuffix(err.Error(), "sendto: can't assign requested address") { - t.Fatal("not the error we expected") - } -} - -func TestDNSDialerInvalidPortSyntax(t *testing.T) { - tlsConf := &tls.Config{NextProtos: []string{"h3"}} - dialer := quicdialer.DNSDialer{ - Resolver: new(net.Resolver), Dialer: &netxlite.QUICDialerQUICGo{ - QUICListener: &netxlite.QUICListenerStdlib{}, - }} - sess, err := dialer.DialContext( - context.Background(), "udp", "www.google.com:port", - tlsConf, &quic.Config{}) - if err == nil { - t.Fatal("expected an error here") - } - if sess != nil { - t.Fatal("expected nil sess") - } - if !errors.Is(err, strconv.ErrSyntax) { - t.Fatal("not the error we expected") - } -} - -func TestDNSDialerDialEarlyFails(t *testing.T) { - tlsConf := &tls.Config{NextProtos: []string{"h3"}} - expected := errors.New("mocked DialEarly error") - dialer := quicdialer.DNSDialer{ - Resolver: new(net.Resolver), Dialer: MockDialer{Err: expected}} - sess, err := dialer.DialContext( - context.Background(), "udp", "www.google.com:443", - tlsConf, &quic.Config{}) - if err == nil { - t.Fatal("expected an error here") - } - if sess != nil { - t.Fatal("expected nil sess") - } - if !errors.Is(err, expected) { - t.Fatal("not the error we expected") - } -} diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index 5a7a3d0..3f53c56 100644 --- a/internal/netxlite/dialer.go +++ b/internal/netxlite/dialer.go @@ -55,7 +55,7 @@ func (d *DialerResolver) DialContext(ctx context.Context, network, address strin } errorslist = append(errorslist, err) } - return nil, ReduceErrors(errorslist) + return nil, reduceErrors(errorslist) } // lookupHost performs a domain name resolution. diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index 670320c..c3af816 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -7,7 +7,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/netx/errorx" ) -// ReduceErrors finds a known error in a list of errors since +// reduceErrors finds a known error in a list of errors since // it's probably most relevant. // // Deprecation warning @@ -16,7 +16,7 @@ import ( // // In perspective, we would like to transition to a scenario where // full dialing is NOT used for measurements and we return a multierror here. -func ReduceErrors(errorslist []error) error { +func reduceErrors(errorslist []error) error { if len(errorslist) == 0 { return nil } diff --git a/internal/netxlite/legacy_test.go b/internal/netxlite/legacy_test.go index 1b8fc21..4132168 100644 --- a/internal/netxlite/legacy_test.go +++ b/internal/netxlite/legacy_test.go @@ -9,14 +9,14 @@ import ( func TestReduceErrors(t *testing.T) { t.Run("no errors", func(t *testing.T) { - result := ReduceErrors(nil) + result := reduceErrors(nil) if result != nil { t.Fatal("wrong result") } }) t.Run("single error", func(t *testing.T) { err := errors.New("mocked error") - result := ReduceErrors([]error{err}) + result := reduceErrors([]error{err}) if result != err { t.Fatal("wrong result") } @@ -24,7 +24,7 @@ func TestReduceErrors(t *testing.T) { t.Run("multiple errors", func(t *testing.T) { err1 := errors.New("mocked error #1") err2 := errors.New("mocked error #2") - result := ReduceErrors([]error{err1, err2}) + result := reduceErrors([]error{err1, err2}) if result.Error() != "mocked error #1" { t.Fatal("wrong result") } @@ -38,7 +38,7 @@ func TestReduceErrors(t *testing.T) { Failure: errorx.FailureConnectionRefused, } err4 := errors.New("mocked error #3") - result := ReduceErrors([]error{err1, err2, err3, err4}) + result := reduceErrors([]error{err1, err2, err3, err4}) if result.Error() != errorx.FailureConnectionRefused { t.Fatal("wrong result") } diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index d02e5ef..f1cea3f 100644 --- a/internal/netxlite/quic.go +++ b/internal/netxlite/quic.go @@ -19,15 +19,6 @@ type QUICContextDialer interface { tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlySession, error) } -// QUICDialer dials QUIC connections. -type QUICDialer interface { - // DialContext establishes a new QUIC session using the given - // network and address. The tlsConfig and the quicConfig arguments - // MUST NOT be nil. Returns either the session or an error. - Dial(network, address string, tlsConfig *tls.Config, - quicConfig *quic.Config) (quic.EarlySession, error) -} - // QUICListener listens for QUIC connections. type QUICListener interface { // Listen creates a new listening PacketConn. @@ -100,3 +91,55 @@ func (sess *quicSessionOwnsConn) CloseWithError( sess.conn.Close() return err } + +// QUICDialerResolver is a dialer that uses the configured Resolver +// to resolve a domain name to IP addrs. +type QUICDialerResolver struct { + // Dialer is the underlying QUIC dialer. + Dialer QUICContextDialer + + // Resolver is the underlying resolver. + Resolver Resolver +} + +// DialContext implements QUICContextDialer.DialContext +func (d *QUICDialerResolver) DialContext( + ctx context.Context, network, address string, + tlsConfig *tls.Config, quicConfig *quic.Config) (quic.EarlySession, error) { + onlyhost, onlyport, err := net.SplitHostPort(address) + if err != nil { + return nil, err + } + // TODO(kelmenhorst): Should this be somewhere else? + // failure if tlsCfg is nil but that should not happen + if tlsConfig.ServerName == "" { + tlsConfig.ServerName = onlyhost + } + addrs, err := d.lookupHost(ctx, onlyhost) + if err != nil { + return nil, err + } + // TODO(bassosimone): here we should be using multierror rather + // than just calling ReduceErrors. We are not ready to do that + // yet, though. To do that, we need first to modify nettests so + // that we actually avoid dialing when measuring. + var errorslist []error + for _, addr := range addrs { + target := net.JoinHostPort(addr, onlyport) + sess, err := d.Dialer.DialContext( + ctx, network, target, tlsConfig, quicConfig) + if err == nil { + return sess, nil + } + errorslist = append(errorslist, err) + } + return nil, reduceErrors(errorslist) +} + +// lookupHost performs a domain name resolution. +func (d *QUICDialerResolver) lookupHost(ctx context.Context, hostname string) ([]string, error) { + if net.ParseIP(hostname) != nil { + return []string{hostname}, nil + } + return d.Resolver.LookupHost(ctx, hostname) +} diff --git a/internal/netxlite/quic_test.go b/internal/netxlite/quic_test.go index 873a279..c1553bb 100644 --- a/internal/netxlite/quic_test.go +++ b/internal/netxlite/quic_test.go @@ -133,3 +133,95 @@ func TestQUICDialerWorksAsIntended(t *testing.T) { log.Fatal(err) } } + +func TestQUICDialerResolverSuccess(t *testing.T) { + tlsConfig := &tls.Config{NextProtos: []string{"h3"}} + dialer := &QUICDialerResolver{ + Resolver: &net.Resolver{}, Dialer: &QUICDialerQUICGo{ + QUICListener: &QUICListenerStdlib{}, + }} + sess, err := dialer.DialContext( + context.Background(), "udp", "www.google.com:443", + tlsConfig, &quic.Config{}) + if err != nil { + t.Fatal(err) + } + <-sess.HandshakeComplete().Done() + if err := sess.CloseWithError(0, ""); err != nil { + t.Fatal(err) + } +} + +func TestQUICDialerResolverNoPort(t *testing.T) { + tlsConfig := &tls.Config{NextProtos: []string{"h3"}} + dialer := &QUICDialerResolver{ + Resolver: new(net.Resolver), Dialer: &QUICDialerQUICGo{}} + sess, err := dialer.DialContext( + context.Background(), "udp", "www.google.com", + tlsConfig, &quic.Config{}) + if err == nil || !strings.HasSuffix(err.Error(), "missing port in address") { + t.Fatal("not the error we expected") + } + if sess != nil { + t.Fatal("expected a nil sess here") + } +} + +func TestQUICDialerResolverLookupHostAddress(t *testing.T) { + dialer := &QUICDialerResolver{Resolver: &netxmocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + // We should not arrive here and call this function but if we do then + // there is going to be an error that fails this test. + return nil, errors.New("mocked error") + }, + }} + addrs, err := dialer.lookupHost(context.Background(), "1.1.1.1") + if err != nil { + t.Fatal(err) + } + if len(addrs) != 1 || addrs[0] != "1.1.1.1" { + t.Fatal("not the result we expected") + } +} + +func TestQUICDialerResolverLookupHostFailure(t *testing.T) { + tlsConfig := &tls.Config{NextProtos: []string{"h3"}} + expected := errors.New("mocked error") + dialer := &QUICDialerResolver{Resolver: &netxmocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return nil, expected + }, + }} + sess, err := dialer.DialContext( + context.Background(), "udp", "dns.google.com:853", + tlsConfig, &quic.Config{}) + if !errors.Is(err, expected) { + t.Fatal("not the error we expected") + } + if sess != nil { + t.Fatal("expected nil sess") + } +} + +func TestQUICDialerResolverInvalidPort(t *testing.T) { + // This test allows us to check for the case where every attempt + // to establish a connection leads to a failure + tlsConf := &tls.Config{NextProtos: []string{"h3"}} + dialer := &QUICDialerResolver{ + Resolver: new(net.Resolver), Dialer: &QUICDialerQUICGo{ + QUICListener: &QUICListenerStdlib{}, + }} + sess, err := dialer.DialContext( + context.Background(), "udp", "www.google.com:0", + tlsConf, &quic.Config{}) + if err == nil { + t.Fatal("expected an error here") + } + if !strings.HasSuffix(err.Error(), "sendto: invalid argument") && + !strings.HasSuffix(err.Error(), "sendto: can't assign requested address") { + t.Fatal("not the error we expected", err) + } + if sess != nil { + t.Fatal("expected nil sess") + } +}