From 2ef5fb503ac9a71aad676147d50cdb498778c62e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 8 Mar 2021 12:05:43 +0100 Subject: [PATCH] fix(webconnectivity): allow measuring https://1.1.1.1 (#241) * fix(webconnectivity): allow measuring https://1.1.1.1 There were two issues preventing us from doing so: 1. in netx, the address resolver was too later in the resolver chain. Therefore, its result wasn't added to the events. 2. when building the DNSCache (in httpget.go), we didn't consider the case where the input is an address. We need to treat this case specially to make sure there is no DNSCache. See https://github.com/ooni/probe/issues/1376. * fix: add unit tests for code making the dnscache * fix(netx): make sure all tests pass * chore: bump webconnectivity version --- .../experiment/webconnectivity/control.go | 4 +- .../experiment/webconnectivity/dnslookup.go | 1 + .../experiment/webconnectivity/httpget.go | 19 ++++- .../webconnectivity/httpget_test.go | 17 ++++ .../webconnectivity/webconnectivity.go | 2 +- .../webconnectivity/webconnectivity_test.go | 2 +- internal/engine/netx/netx.go | 2 +- internal/engine/netx/netx_test.go | 82 +++++++++---------- 8 files changed, 82 insertions(+), 47 deletions(-) diff --git a/internal/engine/experiment/webconnectivity/control.go b/internal/engine/experiment/webconnectivity/control.go index 8d4b3ab..4d189b0 100644 --- a/internal/engine/experiment/webconnectivity/control.go +++ b/internal/engine/experiment/webconnectivity/control.go @@ -57,13 +57,13 @@ func Control( HTTPClient: sess.DefaultHTTPClient(), Logger: sess.Logger(), } - sess.Logger().Infof("control %s...", creq.HTTPRequest) + sess.Logger().Infof("control for %s...", creq.HTTPRequest) // make sure error is wrapped err = errorx.SafeErrWrapperBuilder{ Error: clnt.PostJSON(ctx, "/", creq, &out), Operation: errorx.TopLevelOperation, }.MaybeBuild() - sess.Logger().Infof("control %s... %+v", creq.HTTPRequest, err) + sess.Logger().Infof("control for %s... %+v", creq.HTTPRequest, err) (&out.DNS).FillASNs(sess) return } diff --git a/internal/engine/experiment/webconnectivity/dnslookup.go b/internal/engine/experiment/webconnectivity/dnslookup.go index 92133b7..6d0a324 100644 --- a/internal/engine/experiment/webconnectivity/dnslookup.go +++ b/internal/engine/experiment/webconnectivity/dnslookup.go @@ -37,6 +37,7 @@ func DNSLookup(ctx context.Context, config DNSLookupConfig) (out DNSLookupResult } if answer.IPv6 != "" { out.Addrs[answer.IPv6] = answer.ASN + continue } } } diff --git a/internal/engine/experiment/webconnectivity/httpget.go b/internal/engine/experiment/webconnectivity/httpget.go index ad177f5..35200ca 100644 --- a/internal/engine/experiment/webconnectivity/httpget.go +++ b/internal/engine/experiment/webconnectivity/httpget.go @@ -3,6 +3,7 @@ package webconnectivity import ( "context" "fmt" + "net" "net/url" "strings" @@ -25,6 +26,22 @@ type HTTPGetResult struct { Failure *string } +// TODO(bassosimone): Web Connectivity uses too much external testing +// and we should actually expose much less to the outside by using +// internal testing and by making _many_ functions private. + +// HTTPGetMakeDNSCache constructs the DNSCache option for HTTPGet +// by combining domain and addresses into a single string. As a +// corner case, if the domain is an IP address, we return an empty +// string. This corner case corresponds to Web Connectivity +// inputs like https://1.1.1.1. +func HTTPGetMakeDNSCache(domain, addresses string) string { + if net.ParseIP(domain) != nil { + return "" + } + return fmt.Sprintf("%s %s", domain, addresses) +} + // HTTPGet performs the HTTP/HTTPS part of Web Connectivity. func HTTPGet(ctx context.Context, config HTTPGetConfig) (out HTTPGetResult) { addresses := strings.Join(config.Addresses, " ") @@ -38,7 +55,7 @@ func HTTPGet(ctx context.Context, config HTTPGetConfig) (out HTTPGetResult) { domain := config.TargetURL.Hostname() result, err := urlgetter.Getter{ Config: urlgetter.Config{ - DNSCache: fmt.Sprintf("%s %s", domain, addresses), + DNSCache: HTTPGetMakeDNSCache(domain, addresses), }, Session: config.Session, Target: target, diff --git a/internal/engine/experiment/webconnectivity/httpget_test.go b/internal/engine/experiment/webconnectivity/httpget_test.go index d19cdbf..6f1ddc9 100644 --- a/internal/engine/experiment/webconnectivity/httpget_test.go +++ b/internal/engine/experiment/webconnectivity/httpget_test.go @@ -25,3 +25,20 @@ func TestHTTPGet(t *testing.T) { t.Fatal(*r.Failure) } } + +func TestHTTPGetMakeDNSCache(t *testing.T) { + // test for input being an IP + out := webconnectivity.HTTPGetMakeDNSCache( + "1.1.1.1", "1.1.1.1", + ) + if out != "" { + t.Fatal("expected empty output here") + } + // test for input being a domain + out = webconnectivity.HTTPGetMakeDNSCache( + "dns.google", "8.8.8.8 8.8.4.4", + ) + if out != "dns.google 8.8.8.8 8.8.4.4" { + t.Fatal("expected ordinary output here") + } +} diff --git a/internal/engine/experiment/webconnectivity/webconnectivity.go b/internal/engine/experiment/webconnectivity/webconnectivity.go index 9788e49..7f76ed4 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity.go @@ -19,7 +19,7 @@ import ( const ( testName = "web_connectivity" - testVersion = "0.2.0" + testVersion = "0.3.0" ) // Config contains the experiment config. diff --git a/internal/engine/experiment/webconnectivity/webconnectivity_test.go b/internal/engine/experiment/webconnectivity/webconnectivity_test.go index e9e3335..8a04618 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity_test.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity_test.go @@ -21,7 +21,7 @@ func TestNewExperimentMeasurer(t *testing.T) { if measurer.ExperimentName() != "web_connectivity" { t.Fatal("unexpected name") } - if measurer.ExperimentVersion() != "0.2.0" { + if measurer.ExperimentVersion() != "0.3.0" { t.Fatal("unexpected version") } } diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 554fcdf..25f2365 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -126,6 +126,7 @@ func NewResolver(config Config) Resolver { config.BaseResolver = resolver.SystemResolver{} } var r Resolver = config.BaseResolver + r = resolver.AddressResolver{Resolver: r} if config.CacheResolutions { r = &resolver.CacheResolver{Resolver: r} } @@ -146,7 +147,6 @@ func NewResolver(config Config) Resolver { if config.ResolveSaver != nil { r = resolver.SaverResolver{Resolver: r, Saver: config.ResolveSaver} } - r = resolver.AddressResolver{Resolver: r} return resolver.IDNAResolver{Resolver: r} } diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index 00bf58e..9526ee8 100644 --- a/internal/engine/netx/netx_test.go +++ b/internal/engine/netx/netx_test.go @@ -23,15 +23,15 @@ func TestNewResolverVanilla(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) + ewr, ok := ir.Resolver.(resolver.ErrorWrapperResolver) if !ok { t.Fatal("not the resolver we expected") } - ewr, ok := ar.Resolver.(resolver.ErrorWrapperResolver) + ar, ok := ewr.Resolver.(resolver.AddressResolver) if !ok { t.Fatal("not the resolver we expected") } - _, ok = ewr.Resolver.(resolver.SystemResolver) + _, ok = ar.Resolver.(resolver.SystemResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -47,15 +47,15 @@ func TestNewResolverSpecificResolver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) + ewr, ok := ir.Resolver.(resolver.ErrorWrapperResolver) if !ok { t.Fatal("not the resolver we expected") } - ewr, ok := ar.Resolver.(resolver.ErrorWrapperResolver) + ar, ok := ewr.Resolver.(resolver.AddressResolver) if !ok { t.Fatal("not the resolver we expected") } - _, ok = ewr.Resolver.(resolver.BogonResolver) + _, ok = ar.Resolver.(resolver.BogonResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -69,11 +69,7 @@ func TestNewResolverWithBogonFilter(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ar.Resolver.(resolver.ErrorWrapperResolver) + ewr, ok := ir.Resolver.(resolver.ErrorWrapperResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -81,7 +77,11 @@ func TestNewResolverWithBogonFilter(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = br.Resolver.(resolver.SystemResolver) + ar, ok := br.Resolver.(resolver.AddressResolver) + if !ok { + t.Fatal("not the resolver we expected") + } + _, ok = ar.Resolver.(resolver.SystemResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -95,11 +95,7 @@ func TestNewResolverWithLogging(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - lr, ok := ar.Resolver.(resolver.LoggingResolver) + lr, ok := ir.Resolver.(resolver.LoggingResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -110,7 +106,11 @@ func TestNewResolverWithLogging(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ewr.Resolver.(resolver.SystemResolver) + ar, ok := ewr.Resolver.(resolver.AddressResolver) + if !ok { + t.Fatal("not the resolver we expected") + } + _, ok = ar.Resolver.(resolver.SystemResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -125,11 +125,7 @@ func TestNewResolverWithSaver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - sr, ok := ar.Resolver.(resolver.SaverResolver) + sr, ok := ir.Resolver.(resolver.SaverResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -140,7 +136,11 @@ func TestNewResolverWithSaver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - _, ok = ewr.Resolver.(resolver.SystemResolver) + ar, ok := ewr.Resolver.(resolver.AddressResolver) + if !ok { + t.Fatal("not the resolver we expected") + } + _, ok = ar.Resolver.(resolver.SystemResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -154,11 +154,7 @@ func TestNewResolverWithReadWriteCache(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ar.Resolver.(resolver.ErrorWrapperResolver) + ewr, ok := ir.Resolver.(resolver.ErrorWrapperResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -169,7 +165,11 @@ func TestNewResolverWithReadWriteCache(t *testing.T) { if cr.ReadOnly != false { t.Fatal("expected readwrite cache here") } - _, ok = cr.Resolver.(resolver.SystemResolver) + ar, ok := cr.Resolver.(resolver.AddressResolver) + if !ok { + t.Fatal("not the resolver we expected") + } + _, ok = ar.Resolver.(resolver.SystemResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -185,11 +185,7 @@ func TestNewResolverWithPrefilledReadonlyCache(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - ar, ok := ir.Resolver.(resolver.AddressResolver) - if !ok { - t.Fatal("not the resolver we expected") - } - ewr, ok := ar.Resolver.(resolver.ErrorWrapperResolver) + ewr, ok := ir.Resolver.(resolver.ErrorWrapperResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -203,7 +199,11 @@ func TestNewResolverWithPrefilledReadonlyCache(t *testing.T) { if cr.Get("dns.google.com")[0] != "8.8.8.8" { t.Fatal("cache not correctly prefilled") } - _, ok = cr.Resolver.(resolver.SystemResolver) + ar, ok := cr.Resolver.(resolver.AddressResolver) + if !ok { + t.Fatal("not the resolver we expected") + } + _, ok = ar.Resolver.(resolver.SystemResolver) if !ok { t.Fatal("not the resolver we expected") } @@ -233,7 +233,7 @@ func TestNewDialerVanilla(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - if _, ok := ir.Resolver.(resolver.AddressResolver); !ok { + if _, ok := ir.Resolver.(resolver.ErrorWrapperResolver); !ok { t.Fatal("not the resolver we expected") } ewd, ok := dnsd.Dialer.(dialer.ErrorWrapperDialer) @@ -315,7 +315,7 @@ func TestNewDialerWithLogger(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - if _, ok := ir.Resolver.(resolver.AddressResolver); !ok { + if _, ok := ir.Resolver.(resolver.LoggingResolver); !ok { t.Fatal("not the resolver we expected") } ld, ok := dnsd.Dialer.(dialer.LoggingDialer) @@ -365,7 +365,7 @@ func TestNewDialerWithDialSaver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - if _, ok := ir.Resolver.(resolver.AddressResolver); !ok { + if _, ok := ir.Resolver.(resolver.ErrorWrapperResolver); !ok { t.Fatal("not the resolver we expected") } sad, ok := dnsd.Dialer.(dialer.SaverDialer) @@ -415,7 +415,7 @@ func TestNewDialerWithReadWriteSaver(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - if _, ok := ir.Resolver.(resolver.AddressResolver); !ok { + if _, ok := ir.Resolver.(resolver.ErrorWrapperResolver); !ok { t.Fatal("not the resolver we expected") } scd, ok := dnsd.Dialer.(dialer.SaverConnDialer) @@ -468,7 +468,7 @@ func TestNewDialerWithContextByteCounting(t *testing.T) { if !ok { t.Fatal("not the resolver we expected") } - if _, ok := ir.Resolver.(resolver.AddressResolver); !ok { + if _, ok := ir.Resolver.(resolver.ErrorWrapperResolver); !ok { t.Fatal("not the resolver we expected") } ewd, ok := dnsd.Dialer.(dialer.ErrorWrapperDialer)