From fbe3515691f91ca1140b04f493155a98bbadd521 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 22 Jan 2024 14:14:27 +0100 Subject: [PATCH] feat(webconnectivitylte): handle ghost DNS censorship (#1457) ## 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/2652 - [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: no need - [x] if you changed code inside an experiment, make sure you bump its version number: already bumped for 3.21.x ## Description This commit modifies webconnectivitylte to handle ghost DNS censorship. We define ghost DNS censorship the case where the original domain does not exist anymore but the censor continues to return an IP address for the original domain nonetheless. We used to have null-null handling for this case in the "orig" engine and a reference issue as https://github.com/ooni/probe/issues/2307. With this commit, we modify the "classic" engine to correctly handle this case. To this end, we need to: 1. mark DNS inconsistency when we have successful probe lookups and no IP address has been resolved by the test helper, which is indeed the case of ghost DNS censorship. 2. specific handling on the case in which the website seems down where we also ask ourselves the question of whether the culprit could be the DNS and otherwise set accessible = false and blocking = false. Note that, with those two changes, we depart from strict v0.4-is-always-right orthodoxy. So, while there, let's recognize that always setting HTTPExprimentFailure is probably for the greater good. --- .../webconnectivitylte/analysisclassic.go | 57 ++++++------- .../webconnectivitylte/analysisext.go | 9 ++- .../experiment/webconnectivityqa/badssl.go | 18 ++--- .../webconnectivityqa/dnsblocking.go | 34 ++++---- .../experiment/webconnectivityqa/ghost.go | 81 +++++++++++++++++++ .../experiment/webconnectivityqa/testcase.go | 3 + .../webconnectivityqa/websitedown.go | 17 ++-- 7 files changed, 152 insertions(+), 67 deletions(-) create mode 100644 internal/experiment/webconnectivityqa/ghost.go diff --git a/internal/experiment/webconnectivitylte/analysisclassic.go b/internal/experiment/webconnectivitylte/analysisclassic.go index 4f96e4be2..22e2048bf 100644 --- a/internal/experiment/webconnectivitylte/analysisclassic.go +++ b/internal/experiment/webconnectivitylte/analysisclassic.go @@ -74,7 +74,10 @@ func analysisClassicDNSConsistency(woa *minipipeline.WebAnalysis) optional.Value return optional.Some("consistent") case woa.DNSLookupSuccessWithInvalidAddressesClassic.Len() > 0 || // unexpected addrs; or - woa.DNSLookupUnexpectedFailure.Len() > 0: // unexpected failures + woa.DNSLookupUnexpectedFailure.Len() > 0 || // unexpected failures; or + (woa.DNSLookupSuccess.Len() > 0 && // successful lookups; and + !woa.ControlExpectations.IsNone() && // we have control info; and + woa.ControlExpectations.Unwrap().DNSAddresses.Len() <= 0): // control resolved nothing return optional.Some("inconsistent") default: @@ -106,6 +109,9 @@ type analysisClassicTestKeysProxy interface { // setHTTPExperimentFailure sets the HTTPExperimentFailure field. setHTTPExperimentFailure(value optional.Value[string]) + + // setWebsiteDown sets the test keys for a down website. + setWebsiteDown() } var _ analysisClassicTestKeysProxy = &TestKeys{} @@ -167,6 +173,17 @@ func (tk *TestKeys) setHTTPExperimentFailure(value optional.Value[string]) { tk.HTTPExperimentFailure = value } +// setWebsiteDown implements analysisClassicTestKeysProxy. +func (tk *TestKeys) setWebsiteDown() { + if !tk.DNSConsistency.IsNone() && tk.DNSConsistency.Unwrap() == "inconsistent" { + tk.Blocking = "dns" + tk.Accessible = false + } else { + tk.Blocking = false + tk.Accessible = false + } +} + func analysisClassicComputeBlockingAccessible(woa *minipipeline.WebAnalysis, tk analysisClassicTestKeysProxy) { // minipipeline.NewLinearWebAnalysis produces a woa.Linear sorted // @@ -242,10 +259,7 @@ func analysisClassicComputeBlockingAccessible(woa *minipipeline.WebAnalysis, tk // 2.2. Handle the case where both the probe and the control failed. if entry.ControlHTTPFailure.Unwrap() != "" { - // TODO(bassosimone): returning this result is wrong and we - // should also set Accessible to false. However, v0.4 - // does this and we should play along for the A/B testing. - tk.setBlockingFalse() + tk.setWebsiteDown() tk.setHTTPExperimentFailure(entry.Failure) return } @@ -282,10 +296,7 @@ func analysisClassicComputeBlockingAccessible(woa *minipipeline.WebAnalysis, tk // 3.2. Handle the case where both probe and control failed. if entry.ControlTLSHandshakeFailure.Unwrap() != "" { - // TODO(bassosimone): returning this result is wrong and we - // should set Accessible and Blocking to false. However, v0.4 - // does this and we should play along for the A/B testing. - tk.setBlockingNil() + tk.setWebsiteDown() tk.setHTTPExperimentFailure(entry.Failure) return } @@ -319,10 +330,7 @@ func analysisClassicComputeBlockingAccessible(woa *minipipeline.WebAnalysis, tk // 4.2. Handle the case where both probe and control failed. if entry.ControlTCPConnectFailure.Unwrap() != "" { - // TODO(bassosimone): returning this result is wrong and we - // should set Accessible and Blocking to false. However, v0.4 - // does this and we should play along for the A/B testing. - tk.setBlockingFalse() + tk.setWebsiteDown() tk.setHTTPExperimentFailure(entry.Failure) return } @@ -344,40 +352,27 @@ func analysisClassicComputeBlockingAccessible(woa *minipipeline.WebAnalysis, tk // accessing the website should lead to. if entry.ControlHTTPFailure.IsNone() { tk.setBlockingFalse() - analysisClassicSetHTTPExperimentFailureDNS(tk, entry) + tk.setHTTPExperimentFailure(entry.Failure) return } // 5.1.2. Otherwise, if the control worked, that's blocking. tk.setBlockingString("dns") - analysisClassicSetHTTPExperimentFailureDNS(tk, entry) + tk.setHTTPExperimentFailure(entry.Failure) return } // 5.2. Handle the case where both probe and control failed. if entry.ControlDNSLookupFailure.Unwrap() != "" { - // TODO(bassosimone): returning this result is wrong and we - // should set Accessible and Blocking to false. However, v0.4 - // does this and we should play along for the A/B testing. - tk.setBlockingFalse() - analysisClassicSetHTTPExperimentFailureDNS(tk, entry) + tk.setWebsiteDown() + tk.setHTTPExperimentFailure(entry.Failure) return } // 5.3. Handle the case where just the probe failed. tk.setBlockingString("dns") - analysisClassicSetHTTPExperimentFailureDNS(tk, entry) + tk.setHTTPExperimentFailure(entry.Failure) return } } } - -// analysisClassicSetHTTPExperimentFailureDNS sets the HTTPExperimentFailure if and -// only if the entry's TagDepth is >= 1. We have a v0.4 bug where we do not properly -// set this value for the first HTTP request . -func analysisClassicSetHTTPExperimentFailureDNS(tk analysisClassicTestKeysProxy, entry *minipipeline.WebObservation) { - if entry.TagDepth.UnwrapOr(0) <= 0 { - return - } - tk.setHTTPExperimentFailure(entry.Failure) -} diff --git a/internal/experiment/webconnectivitylte/analysisext.go b/internal/experiment/webconnectivitylte/analysisext.go index 972d75af4..f23d359cd 100644 --- a/internal/experiment/webconnectivitylte/analysisext.go +++ b/internal/experiment/webconnectivitylte/analysisext.go @@ -262,11 +262,14 @@ func analysisExtExpectedFailures(tk *TestKeys, analysis *minipipeline.WebAnalysi // // tk.NullNullFlags |= AnalysisFlagNullNullSuccessfulHTTPS // - // is set by analysisExtHTTPFinalResponse + // is set by analysisExtHTTPFinalResponse when we detect that we do not + // have a control response but nonetheless observe successful HTTPS. - // if the control did not resolve any address but the probe could, this is + // If the control did not resolve any address but the probe could, this is // quite likely censorship injecting addrs for otherwise "down" or nonexisting - // domains, which lives on as a ghost haunting people + // domains, which lives on as a ghost haunting people. + // + // See https://github.com/ooni/probe/issues/2308. if !analysis.ControlExpectations.IsNone() { expect := analysis.ControlExpectations.Unwrap() if expect.DNSAddresses.Len() <= 0 && analysis.DNSLookupSuccess.Len() > 0 { diff --git a/internal/experiment/webconnectivityqa/badssl.go b/internal/experiment/webconnectivityqa/badssl.go index 17e6efc5f..8f6a32121 100644 --- a/internal/experiment/webconnectivityqa/badssl.go +++ b/internal/experiment/webconnectivityqa/badssl.go @@ -10,7 +10,7 @@ import ( func badSSLWithExpiredCertificate() *TestCase { return &TestCase{ Name: "badSSLWithExpiredCertificate", - Flags: 0, + Flags: TestCaseFlagNoV04, Input: "https://expired.badssl.com/", Configure: func(env *netemx.QAEnv) { // nothing @@ -21,8 +21,8 @@ func badSSLWithExpiredCertificate() *TestCase { HTTPExperimentFailure: "ssl_invalid_certificate", XStatus: 16, // StatusAnomalyControlFailure XNullNullFlags: 4, // analysisFlagNullNullTLSMisconfigured - Accessible: nil, - Blocking: nil, + Accessible: false, + Blocking: false, }, } } @@ -32,7 +32,7 @@ func badSSLWithExpiredCertificate() *TestCase { func badSSLWithWrongServerName() *TestCase { return &TestCase{ Name: "badSSLWithWrongServerName", - Flags: 0, + Flags: TestCaseFlagNoV04, Input: "https://wrong.host.badssl.com/", Configure: func(env *netemx.QAEnv) { // nothing @@ -43,8 +43,8 @@ func badSSLWithWrongServerName() *TestCase { HTTPExperimentFailure: "ssl_invalid_hostname", XStatus: 16, // StatusAnomalyControlFailure XNullNullFlags: 4, // analysisFlagNullNullTLSMisconfigured - Accessible: nil, - Blocking: nil, + Accessible: false, + Blocking: false, }, } } @@ -53,7 +53,7 @@ func badSSLWithWrongServerName() *TestCase { func badSSLWithUnknownAuthorityWithConsistentDNS() *TestCase { return &TestCase{ Name: "badSSLWithUnknownAuthorityWithConsistentDNS", - Flags: 0, + Flags: TestCaseFlagNoV04, Input: "https://untrusted-root.badssl.com/", Configure: func(env *netemx.QAEnv) { // nothing @@ -64,8 +64,8 @@ func badSSLWithUnknownAuthorityWithConsistentDNS() *TestCase { HTTPExperimentFailure: "ssl_unknown_authority", XStatus: 16, // StatusAnomalyControlFailure XNullNullFlags: 4, // analysisFlagNullNullTLSMisconfigured - Accessible: nil, - Blocking: nil, + Accessible: false, + Blocking: false, }, } } diff --git a/internal/experiment/webconnectivityqa/dnsblocking.go b/internal/experiment/webconnectivityqa/dnsblocking.go index 207c248a9..3145ee22a 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: 0, + Flags: TestCaseFlagNoV04, Input: "https://www.example.com/", Configure: func(env *netemx.QAEnv) { // make sure the env knows we want to emulate our getaddrinfo wrapper behavior @@ -21,13 +21,14 @@ func dnsBlockingAndroidDNSCacheNoData() *TestCase { }, ExpectErr: false, ExpectTestKeys: &testKeys{ - DNSExperimentFailure: "android_dns_cache_no_data", - DNSConsistency: "inconsistent", - XStatus: 2080, // StatusExperimentDNS | StatusAnomalyDNS - XDNSFlags: 2, // AnalysisDNSFlagUnexpectedFailure - XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess - Accessible: false, - Blocking: "dns", + DNSExperimentFailure: "android_dns_cache_no_data", + HTTPExperimentFailure: "android_dns_cache_no_data", + DNSConsistency: "inconsistent", + XStatus: 2080, // StatusExperimentDNS | StatusAnomalyDNS + XDNSFlags: 2, // AnalysisDNSFlagUnexpectedFailure + XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess + Accessible: false, + Blocking: "dns", }, } } @@ -44,7 +45,7 @@ func dnsBlockingNXDOMAIN() *TestCase { */ return &TestCase{ Name: "dnsBlockingNXDOMAIN", - Flags: 0, + Flags: TestCaseFlagNoV04, Input: "https://www.example.com/", Configure: func(env *netemx.QAEnv) { // remove the record so that the DNS query returns NXDOMAIN @@ -52,13 +53,14 @@ func dnsBlockingNXDOMAIN() *TestCase { }, ExpectErr: false, ExpectTestKeys: &testKeys{ - DNSExperimentFailure: "dns_nxdomain_error", - DNSConsistency: "inconsistent", - XStatus: 2080, // StatusExperimentDNS | StatusAnomalyDNS - XDNSFlags: 2, // AnalysisDNSFlagUnexpectedFailure - XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess - Accessible: false, - Blocking: "dns", + DNSExperimentFailure: "dns_nxdomain_error", + HTTPExperimentFailure: "dns_nxdomain_error", + DNSConsistency: "inconsistent", + XStatus: 2080, // StatusExperimentDNS | StatusAnomalyDNS + XDNSFlags: 2, // AnalysisDNSFlagUnexpectedFailure + XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess + Accessible: false, + Blocking: "dns", }, } } diff --git a/internal/experiment/webconnectivityqa/ghost.go b/internal/experiment/webconnectivityqa/ghost.go new file mode 100644 index 000000000..ed3143229 --- /dev/null +++ b/internal/experiment/webconnectivityqa/ghost.go @@ -0,0 +1,81 @@ +package webconnectivityqa + +import ( + "github.com/apex/log" + "github.com/ooni/netem" + "github.com/ooni/probe-cli/v3/internal/netemx" +) + +// ghostDNSBlockingWithHTTP is the case where the domain does not exist anymore but +// there's still ghost censorship because of the censor DNS censoring configuration, which +// says that we should censor the domain by returning a specific IP address. +// +// See https://github.com/ooni/probe/issues/2308. +func ghostDNSBlockingWithHTTP() *TestCase { + return &TestCase{ + Name: "ghostDNSBlockingWithHTTP", + Flags: TestCaseFlagNoV04, + Input: "http://itsat.info/", + Configure: func(env *netemx.QAEnv) { + // remove the record so that the DNS query returns NXDOMAIN + env.ISPResolverConfig().RemoveRecord("itsat.info") + env.OtherResolversConfig().RemoveRecord("itsat.info") + + // however introduce a rule causing DNS to respond to the query + env.DPIEngine().AddRule(&netem.DPISpoofDNSResponse{ + Addresses: []string{ + netemx.AddressPublicBlockpage, + }, + Logger: log.Log, + Domain: "itsat.info", + }) + }, + ExpectErr: false, + ExpectTestKeys: &testKeys{ + DNSExperimentFailure: nil, + DNSConsistency: "inconsistent", + XBlockingFlags: 16, // AnalysisBlockingFlagHTTPDiff + XNullNullFlags: 16, // AnalysisFlagNullNullUnexpectedDNSLookupSuccess + XStatus: 16, // StatusAnomalyControlFailure + Accessible: false, + Blocking: "dns", + }, + } +} + +// ghostDNSBlockingWithHTTPS is the case where the domain does not exist anymore but +// there's still ghost censorship because of the censor DNS censoring configuration, which +// says that we should censor the domain by returning a specific IP address. +// +// See https://github.com/ooni/probe/issues/2308. +func ghostDNSBlockingWithHTTPS() *TestCase { + return &TestCase{ + Name: "ghostDNSBlockingWithHTTPS", + Flags: 0, + Input: "https://itsat.info/", + Configure: func(env *netemx.QAEnv) { + // remove the record so that the DNS query returns NXDOMAIN + env.ISPResolverConfig().RemoveRecord("itsat.info") + env.OtherResolversConfig().RemoveRecord("itsat.info") + + // however introduce a rule causing DNS to respond to the query + env.DPIEngine().AddRule(&netem.DPISpoofDNSResponse{ + Addresses: []string{ + netemx.AddressPublicBlockpage, + }, + Logger: log.Log, + Domain: "itsat.info", + }) + }, + ExpectErr: false, + ExpectTestKeys: &testKeys{ + DNSExperimentFailure: nil, + DNSConsistency: "inconsistent", + HTTPExperimentFailure: "connection_refused", + XNullNullFlags: 16, // AnalysisFlagNullNullUnexpectedDNSLookupSuccess + XStatus: 4256, // StatusExperimentConnect | StatusAnomalyDNS | StatusAnomalyConnect + Accessible: false, + Blocking: "dns", + }, + } +} diff --git a/internal/experiment/webconnectivityqa/testcase.go b/internal/experiment/webconnectivityqa/testcase.go index 0b0b0b7b0..70151a869 100644 --- a/internal/experiment/webconnectivityqa/testcase.go +++ b/internal/experiment/webconnectivityqa/testcase.go @@ -52,6 +52,9 @@ func AllTestCases() []*TestCase { dnsHijackingToProxyWithHTTPURL(), dnsHijackingToProxyWithHTTPSURL(), + ghostDNSBlockingWithHTTP(), + ghostDNSBlockingWithHTTPS(), + httpBlockingConnectionReset(), httpDiffWithConsistentDNS(), diff --git a/internal/experiment/webconnectivityqa/websitedown.go b/internal/experiment/webconnectivityqa/websitedown.go index aee748d52..e5982db9d 100644 --- a/internal/experiment/webconnectivityqa/websitedown.go +++ b/internal/experiment/webconnectivityqa/websitedown.go @@ -21,18 +21,19 @@ func websiteDownNXDOMAIN() *TestCase { */ return &TestCase{ Name: "websiteDownNXDOMAIN", - Flags: 0, // see above + Flags: TestCaseFlagNoV04, Input: "http://www.example.xyz/", // domain not defined in the simulation Configure: nil, ExpectErr: false, ExpectTestKeys: &testKeys{ - DNSExperimentFailure: "dns_nxdomain_error", - DNSConsistency: "consistent", - XStatus: 2052, // StatusExperimentDNS | StatusSuccessNXDOMAIN - XBlockingFlags: 0, - XNullNullFlags: 1, // analysisFlagNullNullNoAddrs - Accessible: true, - Blocking: false, + DNSExperimentFailure: "dns_nxdomain_error", + HTTPExperimentFailure: "dns_nxdomain_error", + DNSConsistency: "consistent", + XStatus: 2052, // StatusExperimentDNS | StatusSuccessNXDOMAIN + XBlockingFlags: 0, + XNullNullFlags: 1, // analysisFlagNullNullNoAddrs + Accessible: false, + Blocking: false, }, } }