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