Rank rules on evidence, and gate TCP/TLS on the endpoint

oonipipeline/src/oonipipeline/analysis/rules.py

@@ -34,7 +34,23 @@
 from enum import IntEnum
 from typing import List, Tuple
 
-RULES_VERSION = 1
+RULES_VERSION = 2
+
+
+class Evidence(IntEnum):
+    """How much a row has to say about its layer, independent of the score.
+
+    The triple cannot answer this: a rule scores (0, 0, 0) whether the layer
+    was never exercised, or was exercised and then discarded because an
+    earlier layer was untrustworthy. Both are "no verdict", but only the
+    second one names a cause, and neither is "we looked and found nothing
+    wrong". Ordered, so aggregates can prefer the row that saw the most.
+    """
+
+    NONE = 0  # layer produced no data on this row
+    DISCARDED = 1  # observed, but an earlier layer makes it uninterpretable
+    SCORED = 2  # observed and scored
+
 
 
 class Evidence(IntEnum):
@@ -244,15 +260,25 @@ def outcome(self) -> Tuple[float, float, float]:
         ok=0.0,
         comment="Failure against an address that mostly succeeds in the control.",
     ),
+    # Replaces dns_untrusted, which masked on the measurement's DNS verdict and
+    # so discarded every address once any lookup looked poisoned. The question
+    # is about the address, not the measurement: an address the control also
+    # returned, or one that completed a valid handshake, is worth connecting to
+    # whatever the system resolver did. Strictly narrower than the old rule, so
+    # it can only unmask rows, never mask new ones.
     Rule(
-        rule_id="dns_untrusted",
-        condition="dns_blocked > 0 AND dns_ok <= (dns_blocked + dns_down)",
+        rule_id="endpoint_untrusted",
+        condition=(
+            "NOT ip_trusted AND dns_blocked > 0 "
+            "AND dns_ok <= (dns_blocked + dns_down)"
+        ),
         blocked=0.0,
         down=0.0,
         ok=0.0,
         comment=(
-            "DNS was not trustworthy, so the addresses we connected to cannot "
-            "be trusted either. Masked."
+            "The address came from a lookup that looks poisoned and nothing "
+            "independent vouches for it, so connecting to it says nothing "
+            "about the target. Masked."
             # TODO(art): this sits below connect_ok, so a successful connection
             # to a blockpage address is still scored as OK. Is that right?
         ),
@@ -325,13 +351,23 @@ def outcome(self) -> Tuple[float, float, float]:
         ok=0.0,
         comment="Failure where the control succeeds, with a less specific error.",
     ),
+    # See the TCP rule of the same name. Note this sits BELOW the
+    # failure_ctrl_ok_* rules, so a TLS failure against an address the control
+    # succeeds on is already scored as blocking before we get here.
     Rule(
-        rule_id="dns_untrusted",
-        condition="dns_blocked > 0 AND dns_ok <= (dns_blocked + dns_down)",
+        rule_id="endpoint_untrusted",
+        condition=(
+            "NOT ip_trusted AND dns_blocked > 0 "
+            "AND dns_ok <= (dns_blocked + dns_down)"
+        ),
         blocked=0.0,
         down=0.0,
         ok=0.0,
-        comment="DNS was not trustworthy, so this result cannot be either. Masked.",
+        comment=(
+            "The address came from a lookup that looks poisoned and nothing "
+            "independent vouches for it, so the handshake result is not about "
+            "the target. Masked."
+        ),
         evidence=Evidence.DISCARDED,
     ),
     Rule(

oonipipeline/src/oonipipeline/analysis/web_analysis.py

@@ -111,6 +111,25 @@ def format_query_analysis_web_fuzzy_logic(
     --ctrl_tcp_success_rates,
     ctrl_tcp_success_rates[ip] as ctrl_tcp_success_rate,
 
+    -- Is the address in THIS row a real address for this hostname?
+    --
+    -- The TCP and TLS cascades need this to decide whether a result is
+    -- interpretable. Connecting to a censor's blockpage address tells you
+    -- nothing about TCP reachability of the target, and a handshake against it
+    -- tells you nothing about TLS interference -- but that is a property of the
+    -- ADDRESS, not of the measurement. web_connectivity 0.5 tests endpoints
+    -- obtained from resolvers other than the system one, so a poisoned system
+    -- lookup no longer implies every address tested was poisoned.
+    --
+    -- Both signals are independent of whichever resolver produced the address:
+    -- the control resolved it too, or somebody completed a valid handshake for
+    -- the expected name on it. Deliberately NOT including an ASN-level match:
+    -- answer_asn_matches_ctrl is only worth 0.8 ok in the DNS rules, so it is
+    -- not strong enough to license interpreting a failure as censorship.
+    has(mapKeys(ctrl_dns_answers), ip) as ip_in_ctrl_answers,
+    has(union_tls_consistent_ips, ip) as ip_tls_consistent,
+    (ip_in_ctrl_answers OR ip_tls_consistent) as ip_trusted,
+
     expected_countries,
     dns_blocking_scope,
     -- A 'fp' scope marks a fingerprint that is known to produce false
@@ -235,12 +254,24 @@ def format_query_analysis_web_fuzzy_logic(
         dns_failure,
         dns_answer,
 
-        -- We limit this to only the system resolver
-        -- TODO: in order to fully support web_connectivity 0.5 we should ideally
-        -- parse this as well.
-        groupArrayIf(dns_answer, dns_engine IN ('getaddrinfo', 'system')) over (partition by measurement_uid, hostname, ip_is_v6) as dns_answers,
-        groupArrayIf(ip_asn, dns_engine IN ('getaddrinfo', 'system')) over (partition by measurement_uid, hostname, ip_is_v6) as dns_answers_asns,
-        maxIf(ip_is_bogon, dns_engine IN ('getaddrinfo', 'system')) over (partition by measurement_uid, hostname, ip_is_v6) as dns_answers_contain_bogon,
+        -- Each resolver's answers are scored on their own merits, so the
+        -- partition carries the resolver's identity.
+        --
+        -- This used to be restricted to the system resolver and partitioned
+        -- without it, which had two consequences. Answers from the extension
+        -- resolvers web_connectivity 0.5 uses (DNS-over-UDP, DoH, the TH) were
+        -- excluded from DNS scoring entirely; and because a window covers every
+        -- row in its partition, every DNS signal was constant across the
+        -- measurement, so a row whose address came from an untainted resolver
+        -- inherited the system resolver's verdict and was masked with it.
+        --
+        -- groupArray drops NULLs, so a row with no DNS observation at all (an
+        -- HTTP-only redirect hop, or a TH-supplied address) now lands in its own
+        -- partition with an empty answer set and scores no_dns_data, instead of
+        -- borrowing the DNS verdict of a lookup it was not party to.
+        groupArray(dns_answer) over (partition by measurement_uid, hostname, ip_is_v6, dns_engine, dns_engine_resolver_address) as dns_answers,
+        groupArray(ip_asn) over (partition by measurement_uid, hostname, ip_is_v6, dns_engine, dns_engine_resolver_address) as dns_answers_asns,
+        max(ip_is_bogon) over (partition by measurement_uid, hostname, ip_is_v6, dns_engine, dns_engine_resolver_address) as dns_answers_contain_bogon,
 
         countIf(ip_asn IN %(cloud_provider_asns)s) over (partition by measurement_uid) as dns_answers_cloud,
 

oonipipeline/tests/test_rules.py

@@ -290,3 +290,80 @@ def test_top_rule_does_not_rank_on_down_or_ok(layer, rules):
     assert f"{layer}_down" not in sql
     assert f"{layer}_ok" not in sql
     assert f"({layer}_evidence, {layer}_blocked, {layer}_rule_id)" in sql
+
+
+# ------------------------------------------------- per-endpoint trust (wc 0.5)
+
+def test_endpoint_mask_is_narrower_than_the_dns_verdict_it_replaced():
+    """endpoint_untrusted must only ever unmask relative to dns_untrusted.
+
+    The old condition masked on the measurement's DNS verdict alone. The new
+    one conjoins ip_trusted onto it, so every row it masks the old rule masked
+    too. That one-way property is what makes this safe to deploy: no row that
+    was being scored stops being scored.
+    """
+    old = "dns_blocked > 0 AND dns_ok <= (dns_blocked + dns_down)"
+    for layer, rules in LAYER_RULES.items():
+        for rule in rules:
+            if rule.rule_id != "endpoint_untrusted":
+                continue
+            assert "NOT ip_trusted" in rule.condition, layer
+            assert old in rule.condition, (
+                f"{layer}/endpoint_untrusted no longer conjoins the original "
+                f"condition, so it may mask rows dns_untrusted did not")
+
+
+@ALL_LAYERS
+def test_no_rule_still_masks_on_the_bare_dns_verdict(layer, rules):
+    """The whole point of the change: a poisoned system lookup must not
+    discard results for addresses that lookup never produced."""
+    assert "dns_untrusted" not in {r.rule_id for r in rules}
+
+
+def test_endpoint_untrusted_sits_below_the_failure_rules():
+    """Ordering matters more than usual here. A TLS failure against an address
+    the control succeeds on has to be scored as blocking BEFORE we consider
+    masking, or trusting fewer endpoints would lose real positives."""
+    ids = [r.rule_id for r in TLS_RULES]
+    assert ids.index("failure_ctrl_ok_ssl") < ids.index("endpoint_untrusted")
+    assert ids.index("failure_ctrl_ok_reset") < ids.index("endpoint_untrusted")
+    assert ids.index("failure_ctrl_ok_other") < ids.index("endpoint_untrusted")
+    tcp_ids = [r.rule_id for r in TCP_RULES]
+    assert tcp_ids.index("failure_ctrl_ok") < tcp_ids.index("endpoint_untrusted")
+
+
+def test_dns_scoring_is_partitioned_per_resolver():
+    """Every DNS signal is a window over the answer set. If the resolver is not
+    in the partition key the window spans resolvers, and a row whose address
+    came from DoH inherits the system resolver's verdict."""
+    sql, _ = format_query_analysis_web_fuzzy_logic(
+        start_time=__import__("datetime").datetime(2024, 1, 1),
+        end_time=__import__("datetime").datetime(2024, 1, 2),
+        probe_cc=[],
+    )
+    partition = ("partition by measurement_uid, hostname, ip_is_v6, "
+                 "dns_engine, dns_engine_resolver_address")
+    for alias in ("dns_answers", "dns_answers_asns", "dns_answers_contain_bogon"):
+        window = re.search(rf"over \(([^)]*)\) as {alias}\b", sql)
+        assert window, f"{alias} is no longer a window function"
+        assert window.group(1).strip() == partition, (
+            f"{alias} partitions by {window.group(1).strip()!r}, which pools "
+            f"answers across resolvers")
+
+    # The old form restricted the answer set to the system resolver, which
+    # dropped every extension lookup web_connectivity 0.5 performs.
+    assert "groupArrayIf(dns_answer, dns_engine IN" not in sql
+
+
+def test_ip_trusted_is_defined_from_resolver_independent_signals():
+    sql, _ = format_query_analysis_web_fuzzy_logic(
+        start_time=__import__("datetime").datetime(2024, 1, 1),
+        end_time=__import__("datetime").datetime(2024, 1, 2),
+        probe_cc=[],
+    )
+    assert "has(mapKeys(ctrl_dns_answers), ip) as ip_in_ctrl_answers" in sql
+    assert "has(union_tls_consistent_ips, ip) as ip_tls_consistent" in sql
+    assert "(ip_in_ctrl_answers OR ip_tls_consistent) as ip_trusted" in sql
+    # An ASN-level match is not strong enough to license interpreting a
+    # failure as censorship; it must not creep into the trust definition.
+    assert "dns_answer_asn_matches_ctrl) as ip_trusted" not in sql