diff --git a/internal/experiment/webconnectivity/dnsanalysis.go b/internal/experiment/webconnectivity/dnsanalysis.go index dd8f655a35..b3f177498b 100644 --- a/internal/experiment/webconnectivity/dnsanalysis.go +++ b/internal/experiment/webconnectivity/dnsanalysis.go @@ -34,12 +34,14 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult, control ControlResponse) (out DNSAnalysisResult) { // 0. start assuming it's not consistent out.DNSConsistency = &DNSInconsistent + // 1. flip to consistent if we're targeting an IP address because the // control will actually return dns_name_error in this case. if net.ParseIP(URL.Hostname()) != nil { out.DNSConsistency = &DNSConsistent return } + // 2. flip to consistent if the failures are compatible if measurement.Failure != nil && control.DNS.Failure != nil { switch *control.DNS.Failure { @@ -57,6 +59,7 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult, } return } + // 3. flip to consistent if measurement and control returned IP addresses // that belong to the same Autonomous System(s). // @@ -85,6 +88,7 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult, return } } + // 4. when ASN lookup failed (unlikely), check whether // there is overlap in the returned IP addresses ipmap := make(map[string]int) @@ -101,6 +105,7 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult, return } } + // 5. conclude that measurement and control are inconsistent return } diff --git a/internal/experiment/webconnectivity/summary.go b/internal/experiment/webconnectivity/summary.go index 4601a360eb..ef0174aedb 100644 --- a/internal/experiment/webconnectivity/summary.go +++ b/internal/experiment/webconnectivity/summary.go @@ -100,6 +100,7 @@ func Summarize(tk *TestKeys) (out Summary) { defer func() { out.Blocking = DetermineBlocking(out) }() + var ( accessible = true inaccessible = false @@ -108,6 +109,7 @@ func Summarize(tk *TestKeys) (out Summary) { httpFailure = "http-failure" tcpIP = "tcp_ip" ) + // If the measurement was for an HTTPS website and the HTTP experiment // succeeded, then either there is a compromised CA in our pool (which is // certifi-go), or there is transparent proxying, or we are actually @@ -119,11 +121,13 @@ func Summarize(tk *TestKeys) (out Summary) { out.Status |= StatusSuccessSecure return } + // If we couldn't contact the control, we cannot do much more here. if tk.ControlFailure != nil { out.Status |= StatusAnomalyControlUnreachable return } + // If DNS failed with NXDOMAIN and the control DNS is consistent, then it // means this website does not exist anymore. We need to include the weird // cache failure on Android into this analysis because that failure means @@ -142,15 +146,33 @@ func Summarize(tk *TestKeys) (out Summary) { out.Status |= StatusSuccessNXDOMAIN | StatusExperimentDNS return } - // Otherwise, if DNS failed with NXDOMAIN, it's DNS based blocking. - // TODO(bassosimone): do we wanna include other errors here? Like timeout? - if tk.DNSExperimentFailure != nil && - *tk.DNSExperimentFailure == netxlite.FailureDNSNXDOMAINError { + + // Web Connectivity's analysis algorithm up until v0.4.2 gave priority to checking for http-diff + // over saying that there's "dns" blocking when the DNS is inconsistent. + // + // In v0.4.3, we want to address https://github.com/ooni/probe/issues/2499 while still + // trying to preserve the original spirit of the v0.4.2 analysis algorithm. + // + // To this end, we _only_ flag anomaly if the following happens: + // + // 1. the DNS is inconsistent; and + // + // 2. the probe's DNS lookup has failed with dns_nxdomain_error or android_dns_cache_no_data. + // + // By using this algorithm, we narrow the scope and impact of this change but we are, at + // the same time, able to catch cases such as the one mentioned by the issue above. + // + // A more aggressive approach would flag as "dns" blocking any inconsistent result but + // that would depart quite a lot from the behavior of v0.4.2. + if tk.DNSConsistency != nil && *tk.DNSConsistency == DNSInconsistent && + tk.DNSExperimentFailure != nil && (*tk.DNSExperimentFailure == netxlite.FailureDNSNXDOMAINError || + *tk.DNSExperimentFailure == netxlite.FailureAndroidDNSCacheNoData) { out.Accessible = &inaccessible out.BlockingReason = &dns out.Status |= StatusAnomalyDNS | StatusExperimentDNS return } + // If we tried to connect more than once and never succedeed and we were // able to measure DNS consistency, then we can conclude something. if tk.TCPConnectAttempts > 0 && tk.TCPConnectSuccesses <= 0 && tk.DNSConsistency != nil { diff --git a/internal/experiment/webconnectivity/webconnectivity.go b/internal/experiment/webconnectivity/webconnectivity.go index 72b254237d..e19d202e25 100644 --- a/internal/experiment/webconnectivity/webconnectivity.go +++ b/internal/experiment/webconnectivity/webconnectivity.go @@ -15,7 +15,7 @@ import ( const ( testName = "web_connectivity" - testVersion = "0.4.2" + testVersion = "0.4.3" ) // Config contains the experiment config. diff --git a/internal/experiment/webconnectivity/webconnectivity_test.go b/internal/experiment/webconnectivity/webconnectivity_test.go index 69fac2cf72..fad9fe82fa 100644 --- a/internal/experiment/webconnectivity/webconnectivity_test.go +++ b/internal/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.4.2" { + if measurer.ExperimentVersion() != "0.4.3" { t.Fatal("unexpected version") } } diff --git a/internal/experiment/webconnectivityqa/dnsblocking.go b/internal/experiment/webconnectivityqa/dnsblocking.go index b4066905fb..dc27caa9b6 100644 --- a/internal/experiment/webconnectivityqa/dnsblocking.go +++ b/internal/experiment/webconnectivityqa/dnsblocking.go @@ -9,7 +9,7 @@ import ( func dnsBlockingAndroidDNSCacheNoData() *TestCase { return &TestCase{ Name: "dnsBlockingAndroidDNSCacheNoData", - Flags: TestCaseFlagNoV04, // see https://github.com/ooni/probe-cli/pull/1211 + Flags: 0, Input: "https://www.example.com/", Configure: func(env *netemx.QAEnv) { // make sure the env knows we want to emulate our getaddrinfo wrapper behavior @@ -23,8 +23,9 @@ func dnsBlockingAndroidDNSCacheNoData() *TestCase { ExpectTestKeys: &testKeys{ DNSExperimentFailure: "android_dns_cache_no_data", DNSConsistency: "inconsistent", - XDNSFlags: 2, // AnalysisDNSUnexpectedFailure - XBlockingFlags: 33, // analysisFlagDNSBlocking | analysisFlagSuccess + XStatus: 2080, // StatusExperimentDNS | StatusAnomalyDNS + XDNSFlags: 2, // AnalysisDNSUnexpectedFailure + XBlockingFlags: 33, // analysisFlagDNSBlocking | analysisFlagSuccess Accessible: false, Blocking: "dns", }, diff --git a/internal/experiment/webconnectivityqa/run_test.go b/internal/experiment/webconnectivityqa/run_test.go index 0615fe3ba0..d0ae7433cb 100644 --- a/internal/experiment/webconnectivityqa/run_test.go +++ b/internal/experiment/webconnectivityqa/run_test.go @@ -78,7 +78,7 @@ func TestRunTestCase(t *testing.T) { return "web_connectivity" }, MockExperimentVersion: func() string { - return "0.4.2" + return "0.4.3" }, MockRun: func(ctx context.Context, args *model.ExperimentArgs) error { args.Measurement.TestKeys = &testKeys{} @@ -107,7 +107,7 @@ func TestRunTestCase(t *testing.T) { return "web_connectivity" }, MockExperimentVersion: func() string { - return "0.4.2" + return "0.4.3" }, MockRun: func(ctx context.Context, args *model.ExperimentArgs) error { args.Measurement.TestKeys = &testKeys{ @@ -139,7 +139,7 @@ func TestRunTestCase(t *testing.T) { return "web_connectivity" }, MockExperimentVersion: func() string { - return "0.4.2" + return "0.4.3" }, MockRun: func(ctx context.Context, args *model.ExperimentArgs) error { args.Measurement.TestKeys = &testKeys{ @@ -173,7 +173,7 @@ func TestRunTestCase(t *testing.T) { return "web_connectivity" }, MockExperimentVersion: func() string { - return "0.4.2" + return "0.4.3" }, MockRun: func(ctx context.Context, args *model.ExperimentArgs) error { args.Measurement.TestKeys = &testKeys{ diff --git a/internal/experiment/webconnectivityqa/testkeys.go b/internal/experiment/webconnectivityqa/testkeys.go index 91761a3183..f33e8c040c 100644 --- a/internal/experiment/webconnectivityqa/testkeys.go +++ b/internal/experiment/webconnectivityqa/testkeys.go @@ -69,7 +69,7 @@ func compareTestKeys(expected, got *testKeys) error { } switch got.XExperimentVersion { - case "0.4.2": + case "0.4.3": // ignore the fields that are specific to LTE options = append(options, cmpopts.IgnoreFields(testKeys{}, "XDNSFlags", "XBlockingFlags", "XNullNullFlags"))