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:
ooni/probe#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 ooni/probe#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.

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 <facepalm>.
-func analysisClassicSetHTTPExperimentFailureDNS(tk analysisClassicTestKeysProxy, entry *minipipeline.WebObservation) {
-	if entry.TagDepth.UnwrapOr(0) <= 0 {
-		return
-	}
-	tk.setHTTPExperimentFailure(entry.Failure)
-}

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 {

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,
 		},
 	}
 }

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,21 +45,22 @@ 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
 			env.ISPResolverConfig().RemoveRecord("www.example.com")
 		},
 		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",
 		},
 	}
 }

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",
+		},
+	}
+}

internal/experiment/webconnectivityqa/testcase.go

@@ -52,6 +52,9 @@ func AllTestCases() []*TestCase {
 		dnsHijackingToProxyWithHTTPURL(),
 		dnsHijackingToProxyWithHTTPSURL(),
 
+		ghostDNSBlockingWithHTTP(),
+		ghostDNSBlockingWithHTTPS(),
+
 		httpBlockingConnectionReset(),
 
 		httpDiffWithConsistentDNS(),

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,
 		},
 	}
 }