From 23b4371ea79fb6d66e53730052afbe430d5a79f9 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 25 Aug 2023 00:09:33 +0200 Subject: [PATCH] fix(webconnectivity): handle android_dns_cache_no_data as dns anomaly This diff changes the handling of android_dns_cache_no_data to mark it and any other DNS inconsistenct as DNS anomaly. The previous implementation was not handling any DNS inconsistency as an anomaly, therefore I needed to adjust a bunch of summary_test.go tests. Part of https://github.com/ooni/probe/issues/2499 and https://github.com/ooni/probe/issues/2029. I bumped the version number because this is a significant change in the analysis algorithm implemented by the probe. --- internal/experiment/webconnectivity/summary.go | 13 +++++++++---- .../experiment/webconnectivity/summary_test.go | 18 ++++++------------ .../webconnectivity/webconnectivity.go | 2 +- .../webconnectivity/webconnectivity_test.go | 2 +- .../webconnectivityqa/dnsblocking.go | 10 +++++++--- .../experiment/webconnectivityqa/success.go | 12 ++++++++---- .../experiment/webconnectivityqa/testkeys.go | 3 +++ .../webconnectivityqa/tlsblocking.go | 6 ++++-- 8 files changed, 39 insertions(+), 27 deletions(-) diff --git a/internal/experiment/webconnectivity/summary.go b/internal/experiment/webconnectivity/summary.go index 4601a360eb..1e0eb2e1ef 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,16 @@ 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 { + + // Otherwise, if the DNS is inconsistent, flag a DNS failure. Note that this + // case also includes when the probe lookup fails and the TH one does not. + if tk.DNSConsistency != nil && *tk.DNSConsistency == DNSInconsistent { 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/summary_test.go b/internal/experiment/webconnectivity/summary_test.go index 832b61f5ef..85723a2122 100644 --- a/internal/experiment/webconnectivity/summary_test.go +++ b/internal/experiment/webconnectivity/summary_test.go @@ -152,9 +152,7 @@ func TestSummarize(t *testing.T) { BlockingReason: &dns, Blocking: &dns, Accessible: &falseValue, - Status: webconnectivity.StatusAnomalyConnect | - webconnectivity.StatusExperimentConnect | - webconnectivity.StatusAnomalyDNS, + Status: webconnectivity.StatusAnomalyDNS | webconnectivity.StatusExperimentDNS, }, }, { name: "with TCP total failure and unexpected DNS consistency", @@ -350,9 +348,7 @@ func TestSummarize(t *testing.T) { BlockingReason: &dns, Blocking: &dns, Accessible: &falseValue, - Status: webconnectivity.StatusExperimentHTTP | - webconnectivity.StatusAnomalyTLSHandshake | - webconnectivity.StatusAnomalyDNS, + Status: webconnectivity.StatusExperimentDNS | webconnectivity.StatusAnomalyDNS, }, }, { name: "with SSL unknown auth _and_ untrustworthy DNS _and_ a longer chain", @@ -367,11 +363,10 @@ func TestSummarize(t *testing.T) { }, }, wantOut: webconnectivity.Summary{ - BlockingReason: &httpFailure, - Blocking: &httpFailure, + BlockingReason: &dns, + Blocking: &dns, Accessible: &falseValue, - Status: webconnectivity.StatusExperimentHTTP | - webconnectivity.StatusAnomalyTLSHandshake, + Status: webconnectivity.StatusExperimentDNS | webconnectivity.StatusAnomalyDNS, }, }, { name: "with status code and body length matching", @@ -442,8 +437,7 @@ func TestSummarize(t *testing.T) { BlockingReason: &dns, Blocking: &dns, Accessible: &falseValue, - Status: webconnectivity.StatusAnomalyHTTPDiff | - webconnectivity.StatusAnomalyDNS, + Status: webconnectivity.StatusExperimentDNS | webconnectivity.StatusAnomalyDNS, }, }, { name: "with suspect http-diff and consistent DNS", diff --git a/internal/experiment/webconnectivity/webconnectivity.go b/internal/experiment/webconnectivity/webconnectivity.go index 40732abcb9..815b385209 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 6b655ba655..4bb720094d 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 47c236cdf5..2a8b5edd1f 100644 --- a/internal/experiment/webconnectivityqa/dnsblocking.go +++ b/internal/experiment/webconnectivityqa/dnsblocking.go @@ -9,7 +9,7 @@ import ( func dnsBlockingAndroidDNSCacheNoData() *TestCase { return &TestCase{ Name: "measuring https://www.example.com/ with getaddrinfo errors and android_dns_cache_no_data", - Flags: TestCaseFlagNoV04, + 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 @@ -19,7 +19,11 @@ func dnsBlockingAndroidDNSCacheNoData() *TestCase { // converted into android_dns_cache_no_data by the emulation layer env.ISPResolverConfig().RemoveRecord("www.example.com") }, - ExpectErr: false, - ExpectTestKeys: &testKeys{Accessible: false, Blocking: "dns"}, + ExpectErr: false, + ExpectTestKeys: &testKeys{ + DNSConsistency: "inconsistent", + Accessible: false, + Blocking: "dns", + }, } } diff --git a/internal/experiment/webconnectivityqa/success.go b/internal/experiment/webconnectivityqa/success.go index 83d75affdc..477d53923f 100644 --- a/internal/experiment/webconnectivityqa/success.go +++ b/internal/experiment/webconnectivityqa/success.go @@ -4,12 +4,14 @@ package webconnectivityqa func sucessWithHTTP() *TestCase { return &TestCase{ Name: "measuring http://www.example.com/ without censorship", + Flags: 0, Input: "http://www.example.com/", Configure: nil, ExpectErr: false, ExpectTestKeys: &testKeys{ - Accessible: true, - Blocking: false, + DNSConsistency: "consistent", + Accessible: true, + Blocking: false, }, } } @@ -18,12 +20,14 @@ func sucessWithHTTP() *TestCase { func sucessWithHTTPS() *TestCase { return &TestCase{ Name: "measuring https://www.example.com/ without censorship", + Flags: 0, Input: "https://www.example.com/", Configure: nil, ExpectErr: false, ExpectTestKeys: &testKeys{ - Accessible: true, - Blocking: false, + DNSConsistency: "consistent", + Accessible: true, + Blocking: false, }, } } diff --git a/internal/experiment/webconnectivityqa/testkeys.go b/internal/experiment/webconnectivityqa/testkeys.go index bcb5145166..b37aa9719e 100644 --- a/internal/experiment/webconnectivityqa/testkeys.go +++ b/internal/experiment/webconnectivityqa/testkeys.go @@ -11,6 +11,9 @@ import ( // testKeys is the test keys structure returned by this package. type testKeys struct { + // DNSConsistency indicates whether the DNS is consistent with the TH. + DNSConsistency any `json:"dns_consistency"` + // Accessible indicates whether the URL was accessible. Accessible any `json:"accessible"` diff --git a/internal/experiment/webconnectivityqa/tlsblocking.go b/internal/experiment/webconnectivityqa/tlsblocking.go index c178867380..d400fba5b7 100644 --- a/internal/experiment/webconnectivityqa/tlsblocking.go +++ b/internal/experiment/webconnectivityqa/tlsblocking.go @@ -10,6 +10,7 @@ import ( func tlsBlockingConnectionReset() *TestCase { return &TestCase{ Name: "measuring https://www.example.com/ with TLS blocking of the SNI", + Flags: 0, Input: "https://www.example.com/", Configure: func(env *netemx.QAEnv) { env.DPIEngine().AddRule(&netem.DPIResetTrafficForTLSSNI{ @@ -19,8 +20,9 @@ func tlsBlockingConnectionReset() *TestCase { }, ExpectErr: false, ExpectTestKeys: &testKeys{ - Accessible: false, - Blocking: "http-failure", + DNSConsistency: "consistent", + Accessible: false, + Blocking: "http-failure", }, } }