From 29e8c888ba44e7b7a0f80fc5e1f974532ae6d1e0 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 10 Oct 2023 16:07:13 +0200 Subject: [PATCH] fix: anomaly with android_dns_cache_no_data and inconsistent dns (#1211) ## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: https://github.com/ooni/probe/issues/2499 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: N/A - [x] if you changed code inside an experiment, make sure you bump its version number ## Summary This diff changes the `summary.go` algorithm of Web Connectivity v0.4 to handle `android_dns_cache_no_data` as an anomaly when the DNS is inconsistent. We continue handling `dns_nxdomain_error` as an anomaly when the DNS is inconsistent, as demonstrated by the fact that the corresponding netem test is still passing. This diff also bumps the version number to v0.4.3. Version v0.4.2 did not handle this case, which caused measurements to be marked as failed as documented by https://github.com/ooni/probe/issues/2499. This diff is also related to https://github.com/ooni/probe/issues/2029, in the sense that it is slightly improving our analysis results when the is an NXDOMAIN error (even if it's masked by Android's DNS cache behavior). While there, add empty lines to improve the code readability. --- .../experiment/webconnectivity/dnsanalysis.go | 5 ++++ .../experiment/webconnectivity/summary.go | 30 ++++++++++++++++--- .../webconnectivity/webconnectivity.go | 2 +- .../webconnectivity/webconnectivity_test.go | 2 +- .../webconnectivityqa/dnsblocking.go | 7 +++-- .../experiment/webconnectivityqa/run_test.go | 8 ++--- .../experiment/webconnectivityqa/testkeys.go | 2 +- 7 files changed, 42 insertions(+), 14 deletions(-) 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"))