From f45bf404255abe8313f41912f3d1e8cf1c98843f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arturo=20Filast=C3=B2?= Date: Wed, 5 Aug 2026 20:11:01 +0200 Subject: [PATCH] Rank rules on evidence, and gate TCP/TLS on the endpoint Two corrections to the scoring cascade. Both change which rule id is recorded rather than the scores themselves. top_*_rule_id was argMax(rule_id, blocked). On a measurement that is not blocked the key is 0 on every row, so argMax kept whichever row it saw first. Every web_connectivity measurement carries one row per resolved IP plus one per redirect hop, and the redirect rows hold only HTTP, so they scored no_tls_data and were winning that tie. Rules now carry an Evidence level and are ranked on it before the score. DNS scoring was restricted to the system resolver and its window partitioned without the resolver, so every DNS signal was constant across a measurement and a row whose address came from another resolver inherited the system resolver's verdict. dns_untrusted is replaced by endpoint_untrusted, which additionally requires that nothing independent vouches for the address: either the control returned it too, or somebody completed a valid handshake for the expected name on it. RULES_VERSION goes to 2, so rows written before and after are distinguishable. Measured over a 10-minute production window (9,731 measurements), rescoring with the new code moves no *_blocked score and three rule ids, all tcp: endpoint_untrusted -> none. The new condition is strictly narrower than the one it replaces, so it can only unmask rows, never mask new ones. It is close to a no-op until web_connectivity 0.5 measurements reach the pipeline, which is what makes it safe to land first. Reprocessing steps are in the migration notes. --- .../src/oonipipeline/analysis/rules.py | 52 +++++++++++-- .../src/oonipipeline/analysis/web_analysis.py | 43 +++++++++-- oonipipeline/tests/test_rules.py | 77 +++++++++++++++++++ 3 files changed, 158 insertions(+), 14 deletions(-) diff --git a/oonipipeline/src/oonipipeline/analysis/rules.py b/oonipipeline/src/oonipipeline/analysis/rules.py index cbd445b8..2369f1d2 100644 --- a/oonipipeline/src/oonipipeline/analysis/rules.py +++ b/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( diff --git a/oonipipeline/src/oonipipeline/analysis/web_analysis.py b/oonipipeline/src/oonipipeline/analysis/web_analysis.py index aa852cd4..a9b49983 100644 --- a/oonipipeline/src/oonipipeline/analysis/web_analysis.py +++ b/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, diff --git a/oonipipeline/tests/test_rules.py b/oonipipeline/tests/test_rules.py index 5de6c986..45fb16f8 100644 --- a/oonipipeline/tests/test_rules.py +++ b/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