From 8d25a65d8eb13f4b457e3b29b8dd38f040ec4f74 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Mon, 27 Nov 2023 15:12:40 +0100
Subject: [PATCH] fix tricky case with order of DNS processing

(I am thankful there's a ~comprehensive test suite.)
---
 .../webconnectivityqa/dnshijacking.go          | 12 ++++++------
 internal/minipipeline/analysis.go              | 18 ++++++++++++++++--
 internal/minipipeline/observation.go           | 18 ++++++++++++++----
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/internal/experiment/webconnectivityqa/dnshijacking.go b/internal/experiment/webconnectivityqa/dnshijacking.go
index df2264ab3..612d9ab98 100644
--- a/internal/experiment/webconnectivityqa/dnshijacking.go
+++ b/internal/experiment/webconnectivityqa/dnshijacking.go
@@ -27,17 +27,17 @@ func dnsHijackingToProxyWithHTTPURL() *TestCase {
 		},
 		ExpectErr: false,
 		ExpectTestKeys: &testKeys{
-			DNSConsistency:  "inconsistent",
+			DNSConsistency:  "consistent",
 			BodyLengthMatch: true,
 			BodyProportion:  1,
 			StatusCodeMatch: true,
 			HeadersMatch:    true,
 			TitleMatch:      true,
-			XStatus:         2,     // StatusSuccessCleartext
-			XDNSFlags:       4,     // AnalysisDNSUnexpectedAddrs
-			XBlockingFlags:  33,    // AnalysisFlagDNSBlocking | analysisFlagSuccess
-			Accessible:      false, // FIXME: this is probably incorrect!
-			Blocking:        "dns",
+			XStatus:         2, // StatusSuccessCleartext
+			XDNSFlags:       0,
+			XBlockingFlags:  32, // analysisFlagSuccess
+			Accessible:      true,
+			Blocking:        false,
 		},
 	}
 }
diff --git a/internal/minipipeline/analysis.go b/internal/minipipeline/analysis.go
index 152ce905f..dd5a77d31 100644
--- a/internal/minipipeline/analysis.go
+++ b/internal/minipipeline/analysis.go
@@ -203,13 +203,12 @@ func (wa *WebAnalysis) ComputeDNSPossiblyInvalidAddrs(c *WebObservationsContaine
 
 	state := make(map[string]bool)
 
+	// pass 1: insert candidates into the state map
 	for _, obs := range c.KnownTCPEndpoints {
 		addr := obs.IPAddress.Unwrap()
 
 		// if we have a succesful TLS handshake for this addr, we're good
 		if obs.TLSHandshakeFailure.UnwrapOr("unknown_failure") == "" {
-			// just in case another transaction succeded, clear the address from the state
-			delete(state, addr)
 			continue
 		}
 
@@ -227,6 +226,21 @@ func (wa *WebAnalysis) ComputeDNSPossiblyInvalidAddrs(c *WebObservationsContaine
 		state[addr] = true
 	}
 
+	// pass 2: remove IP addresses we could validate using TLS handshakes
+	//
+	// we need to perform this second step because the order with which we walk
+	// through c.KnownTCPEndpoints is not fixed _and_ in any case, there is no
+	// guarantee that we'll observe 80/tcp entries _before_ 443/tcp ones. So, by
+	// applying this algorithm as a second step, we ensure that we're always
+	// able to remove TLS-validate addresses from the "bad" set.
+	for _, obs := range c.KnownTCPEndpoints {
+		addr := obs.IPAddress.Unwrap()
+		if obs.TLSHandshakeFailure.UnwrapOr("") != "" {
+			continue
+		}
+		delete(state, addr)
+	}
+
 	wa.DNSPossiblyInvalidAddrs = optional.Some(state)
 }
 
diff --git a/internal/minipipeline/observation.go b/internal/minipipeline/observation.go
index 43b3fc9c2..298f0b0ab 100644
--- a/internal/minipipeline/observation.go
+++ b/internal/minipipeline/observation.go
@@ -368,15 +368,25 @@ func (c *WebObservationsContainer) controlMatchDNSLookupResults(inputDomain stri
 
 	// walk through the list of known observations
 	for _, obs := range c.KnownTCPEndpoints {
-		// skip entries using a different domain than the one used by the TH
+		// obtain the domain from which we obtained the endpoint's address
 		domain := obs.DNSDomain.UnwrapOr("")
-		if domain == "" || domain != inputDomain {
-			continue
-		}
 
 		// obtain the IP address
 		addr := obs.IPAddress.Unwrap()
 
+		// handle the case from which the IP address has been provided by the control, which
+		// is a case where the domain is empty and the IP address is in thAddrMap
+		if domain == "" && thAddrMap[addr] {
+			obs.MatchWithControlIPAddress = optional.Some(true)
+			obs.MatchWithControlIPAddressASN = optional.Some(true)
+			continue
+		}
+
+		// skip entries using a different domain than the one used by the TH
+		if domain == "" || domain != inputDomain {
+			continue
+		}
+
 		// compute whether also the TH observed this addr
 		obs.MatchWithControlIPAddress = optional.Some(thAddrMap[addr])