review fixes

src/backend/core/services/dns/check.py

@@ -10,6 +10,7 @@ from typing import Dict, List, Optional, Tuple
 from django.core.cache import cache
 
 import dns.resolver
+from sentry_sdk import capture_exception
 
 from core.models import MailDomain
 
@@ -30,22 +31,24 @@ DKIM_TAG_NAME_RE = re.compile(r"[a-zA-Z]")
 SPF_VERSION = "v=spf1"
 # RFC 7208 4.5: a record starts with a version section of exactly "v=spf1",
 # terminated by a space or the end of the record, so "v=spf10" is not one.
-# Per Section 12, ABNF literals are case-insensitive: "V=sPf1" is.
-SPF_VERSION_RE = re.compile(rf"{SPF_VERSION}(\s|$)", re.IGNORECASE)
+# Per Section 12, ABNF literals are case-insensitive: "V=sPf1" is. Only a
+# space terminates it: RFC 7208 4.6.1 separates terms with SP alone, and a
+# record broken by another control character is one receivers reject too.
+SPF_VERSION_RE = re.compile(rf"{SPF_VERSION}( |$)", re.IGNORECASE)
 # RFC 7208 4.6.1: directive = [ qualifier ] mechanism.
 SPF_QUALIFIERS = "+-?~"
 # RFC 7208 4.6.1: a term name ends at the first ":", "=" or "/".
 SPF_TERM_NAME_RE = re.compile(r"[^:=/]*")
-SPF_ALL_MECHANISMS = frozenset({"+all", "-all", "~all", "?all"})
 # Ordered from most permissive to strictest (RFC 7208 4.6.2).
 SPF_ALL_STRICTNESS = {"+all": 0, "?all": 1, "~all": 2, "-all": 3}
+SPF_ALL_MECHANISMS = frozenset(SPF_ALL_STRICTNESS)
 
 # RFC 7208 3.3 and RFC 6376 3.6.2: the strings of a single TXT record are
 # concatenated with no separator, which is how records over 255 octets are
 # published. Some local resolvers (e.g. systemd-resolved) instead merge
 # separate TXT records into one RR; those show up as a later string opening
 # its own record, and have to stay apart.
-TXT_RECORD_START_RE = re.compile(r"v=(spf1(\s|$)|DMARC1\b)", re.IGNORECASE)
+TXT_RECORD_START_RE = re.compile(r"v=(spf1( |$)|DMARC1\b)", re.IGNORECASE)
 
 
 def normalize_txt_value(value: str) -> str:
@@ -134,8 +137,10 @@ def parse_spf_terms(value: str) -> Optional[Tuple[str, set]]:
 
     Returns (all_mechanism, other_terms) where all_mechanism is the canonical
     "-all", "~all", "+all" or "?all" (a bare "all" means "+all"), or None when
-    the record has no "all" at all. Terms are canonicalized, so ordering,
-    letter case and implicit qualifiers do not matter.
+    the record has no "all" at all. Only the first one counts: "all" always
+    matches, so anything after it is never tested (RFC 7208 5.1). Terms are
+    canonicalized, so ordering, letter case and implicit qualifiers do not
+    matter.
     Returns None if not a valid SPF record.
     """
     if not is_spf_record(value):
@@ -145,7 +150,8 @@ def parse_spf_terms(value: str) -> Optional[Tuple[str, set]]:
     for term in value[len(SPF_VERSION) :].split():
         canonical = _canonical_spf_term(term)
         if canonical in SPF_ALL_MECHANISMS:
-            all_mechanism = canonical
+            if all_mechanism is None:
+                all_mechanism = canonical
         else:
             other_terms.add(canonical)
     return (all_mechanism, other_terms)
@@ -202,13 +208,15 @@ def _check_spf(expected_value: str, found_values: List[str]) -> Dict[str, any]:
     # If there are expected includes, check they resolve via BFS.
     # This is the primary signal: includes being set up is what matters.
     if expected_includes:
-        resolved, transient, error = _resolve_spf_includes(found_spf_values)
+        resolved, visited, transient, error = _resolve_spf_includes(found_spf_values)
         if not expected_includes <= resolved:
             # A problem met while walking the chain only matters when it is
             # what kept our own include out of reach: a third party
             # duplicating its record, or a chain too long past our include,
             # says nothing about the record we asked the customer to publish.
-            if transient:
+            # A branch that failed transiently is unexplored, so unless every
+            # include we expect was looked up and settled, it may sit there.
+            if transient and not expected_includes <= visited - transient:
                 return {
                     "status": "error",
                     "error": "DNS query failed while following the SPF chain",
@@ -265,37 +273,42 @@ def _extract_include_domains(spf_value: str) -> List[str]:
 
     Both the "include:" mechanism and the "redirect=" modifier hand the
     decision over to another domain's record (RFC 7208 5.2 and 6.1), so a
-    domain reached either way counts.
+    domain reached either way counts. An "all" mechanism anywhere in the
+    record makes the redirect inoperative though (RFC 7208 5.1 and 6.1), and
+    a record it never reaches delegates nothing.
     """
-    domains = []
+    includes = []
+    redirects = []
+    has_all = False
     for term in spf_value.split():
         qualifier, name, argument = _parse_spf_term(term)
-        if name == "include" and argument.startswith(":"):
-            domain = argument[1:]
+        if name == "all" and not argument:
+            has_all = True
+        elif name == "include" and argument.startswith(":"):
+            includes.append(argument[1:])
         elif name == "redirect" and not qualifier and argument.startswith("="):
-            domain = argument[1:]
-        else:
-            continue
-        # A macro only expands at evaluation time, so it names no domain we
-        # could look up here.
-        if domain and "%" not in domain:
-            domains.append(domain)
-    return domains
+            redirects.append(argument[1:])
+    # A redirect is only reached once every mechanism failed to match, so it
+    # comes last. A macro only expands at evaluation time, so it names no
+    # domain we could look up here.
+    domains = includes if has_all else includes + redirects
+    return [domain for domain in domains if domain and "%" not in domain]
 
 
 def _resolve_spf_includes(
     found_values: List[str], max_lookups: int = 10
-) -> Tuple[set, set, Optional[str]]:
+) -> Tuple[set, set, set, Optional[str]]:
     """BFS through SPF include chains, return all domains with valid SPF records.
 
     Seeds from the domains found_values delegate to, follows the chain via BFS.
     Per RFC 7208, stops after max_lookups DNS lookups.
 
     Returns:
-        (resolved_domains, transient_failures, error) where transient_failures
-        holds the domains whose lookup failed in a way that may well succeed
-        next time, and error is None on success, or a string describing the
-        issue ("limit_reached", "duplicate:domain.com").
+        (resolved_domains, visited_domains, transient_failures, error) where
+        visited_domains are the ones we got to look up, transient_failures the
+        ones whose lookup failed in a way that may well succeed next time, and
+        error is None on success, or a string describing the issue
+        ("limit_reached", "duplicate:domain.com").
     """
     queue = collections.deque()
     for found_value in found_values:
@@ -309,7 +322,7 @@ def _resolve_spf_includes(
 
     while queue:
         if lookup_count >= max_lookups:
-            return resolved, transient, "limit_reached"
+            return resolved, visited, transient, "limit_reached"
 
         include_domain = queue.popleft()
         if include_domain in visited:
@@ -325,16 +338,24 @@ def _resolve_spf_includes(
                 for value in _txt_record_values(rr)
                 if is_spf_record(value)
             ]
+        except (dns.resolver.NXDOMAIN, dns.resolver.NoAnswer):
+            # An include pointing at a name that publishes nothing is a
+            # settled answer, not a failure to look it up.
+            logger.debug("No TXT record for %s", include_domain)
+            continue
         except (dns.resolver.Timeout, dns.resolver.NoNameservers):
-            logger.debug("DNS resolution timed out for %s", include_domain)
+            logger.debug("DNS resolution failed for %s, may retry", include_domain)
             transient.add(include_domain)
             continue
-        except Exception:  # pylint: disable=broad-exception-caught
-            logger.debug("DNS resolution failed for %s", include_domain)
+        except Exception as exc:  # pylint: disable=broad-exception-caught
+            logger.warning(
+                "Unexpected error resolving %s: %s", include_domain, exc, exc_info=True
+            )
+            capture_exception(exc)
             continue
 
         if len(spf_records) > 1:
-            return resolved, transient, f"duplicate:{include_domain}"
+            return resolved, visited, transient, f"duplicate:{include_domain}"
 
         if not spf_records:
             continue
@@ -344,7 +365,7 @@ def _resolve_spf_includes(
             if child_domain not in visited:
                 queue.append(child_domain)
 
-    return resolved, transient, None
+    return resolved, visited, transient, None
 
 
 def _txt_record_values(rr) -> List[str]:
@@ -352,9 +373,11 @@ def _txt_record_values(rr) -> List[str]:
 
     Its strings belong to one record and are concatenated, unless a later one
     opens a record of its own — the sign of a resolver having merged separate
-    records into one RR.
+    records into one RR. SPF records are US-ASCII (RFC 7208 3.1), but an
+    unrelated TXT record at the same name may hold anything, and that is no
+    reason to fail the whole check.
     """
-    strings = [s.decode() for s in rr.strings]
+    strings = [s.decode(errors="replace") for s in rr.strings]
     if any(TXT_RECORD_START_RE.match(s) for s in strings[1:]):
         return [normalize_txt_value(s) for s in strings]
     return [normalize_txt_value("".join(strings))]
@@ -372,7 +395,9 @@ def _resolve_dns_values(record_type, target, query_name):
         for rr in answers.rrset:
             if target.endswith("._domainkey"):
                 # DKIM: concatenate strings (long key split across strings)
-                values.append(normalize_txt_value(b"".join(rr.strings).decode()))
+                values.append(
+                    normalize_txt_value(b"".join(rr.strings).decode(errors="replace"))
+                )
             else:
                 values.extend(_txt_record_values(rr))
         return values
@@ -383,17 +408,12 @@ def _resolve_dns_values(record_type, target, query_name):
 
 def _check_txt_security(expected_value, found_values):
     """Check for duplicate/insecure SPF and DMARC records. Returns result or None."""
-    # SPF duplicate and insecure checks
+    # SPF duplicate check. Whether the policy is strong enough is left to
+    # _check_spf: a weak "all" only reads as "insecure" once the delegation
+    # is known to be in place, and "insecure" is a status we still send on.
     if is_spf_record(expected_value):
-        spf_records = [v for v in found_values if is_spf_record(v)]
-        if len(spf_records) > 1:
+        if len([v for v in found_values if is_spf_record(v)]) > 1:
             return {"status": "duplicate", "found": found_values}
-        expected_all, _ = parse_spf_terms(expected_value)
-        if expected_all == "-all":
-            for spf in spf_records:
-                found_all, _ = parse_spf_terms(spf)
-                if found_all in ("+all", "?all"):
-                    return {"status": "insecure", "found": found_values}
 
     # DMARC duplicate and insecure checks
     if expected_value.startswith("v=DMARC1"):
@@ -502,7 +522,9 @@ def _check_spf_status_uncached(maildomain: MailDomain) -> Tuple[bool, bool]:
     spf_records = [
         r
         for r in expected_records
-        if r["type"].upper() == "TXT" and is_spf_record(r["value"])
+        # check_single_record normalizes before matching, so a configured
+        # value carrying its zone-file quotes has to be recognized here too.
+        if r["type"].upper() == "TXT" and is_spf_record(normalize_txt_value(r["value"]))
     ]
     if not spf_records:
         return True, True

src/backend/core/tests/dns/test_check.py

@@ -949,6 +949,18 @@ class TestParseSpfTerms:
         """RFC 7208 4.5: "v=spf10" is not an SPF record."""
         assert parse_spf_terms("v=spf10 include:_spf.example.com -all") is None
 
+    def test_version_must_be_terminated_by_a_space(self):
+        """RFC 7208 4.6.1 separates terms with SP alone. Receivers read a
+        record broken by another control character as no record at all, so we
+        must not report it as one either."""
+        assert parse_spf_terms("v=spf1\tinclude:_spf.example.com -all") is None
+
+    def test_empty_record_is_valid(self):
+        """RFC 7208 4.5: a bare "v=spf1" is a record, with no terms."""
+        all_mech, terms = parse_spf_terms("v=spf1")
+        assert all_mech is None
+        assert terms == set()
+
     def test_qualifiers_are_made_explicit(self):
         """An omitted qualifier means "+", so both spellings are one term."""
         _all_mech, terms = parse_spf_terms("v=spf1 +MX ip4:1.2.3.4")
@@ -959,6 +971,12 @@ class TestParseSpfTerms:
         all_mech, _terms = parse_spf_terms("v=spf1 mx all")
         assert all_mech == "+all"
 
+    def test_first_all_wins(self):
+        """RFC 7208 5.1: "all" always matches, so anything after it is never
+        tested — including a second, stricter "all"."""
+        all_mech, _terms = parse_spf_terms("v=spf1 +all -all")
+        assert all_mech == "+all"
+
     def test_modifiers_keep_no_qualifier(self):
         """Modifiers are name=value pairs and take no qualifier."""
         _all_mech, terms = parse_spf_terms("v=spf1 redirect=_spf.example.com")
@@ -1456,7 +1474,7 @@ class TestSPFValidRecordsAreNotFlagged:
 
             result = check_single_record(maildomain, expected_record)
             # The delegation is in place; only the "all" is missing locally.
-            assert result["status"] in ("correct", "insecure")
+            assert result["status"] == "insecure"
 
     def test_v_spf10_is_not_a_second_record(self, maildomain_factory):
         """RFC 7208 4.5: the version section is terminated by a space or the end
@@ -1537,6 +1555,35 @@ class TestSPFValidRecordsAreNotFlagged:
             result = check_single_record(maildomain, expected_record)
             assert result["status"] == "correct"
 
+    def test_unrelated_non_ascii_txt_record(self, maildomain_factory):
+        """SPF records are US-ASCII (RFC 7208 3.1), but another TXT record at
+        the same name may hold anything, and decoding it must not fail the
+        whole check."""
+        maildomain = maildomain_factory(name="example.com")
+        expected_record = {
+            "type": "TXT",
+            "target": "",
+            "value": "v=spf1 include:_spf.example.com -all",
+        }
+
+        binary_rr = MagicMock()
+        binary_rr.strings = (b"\xff\xfe some vendor blob",)
+        spf_rr = MagicMock()
+        spf_rr.strings = (b"v=spf1 include:_spf.example.com -all",)
+        answer = MagicMock()
+        answer.rrset = [binary_rr, spf_rr]
+
+        with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve:
+            mock_resolve.side_effect = self._resolver(
+                {
+                    "example.com": answer,
+                    "_spf.example.com": _txt_answer("v=spf1 ip4:1.2.3.4 -all"),
+                }
+            )
+
+            result = check_single_record(maildomain, expected_record)
+            assert result["status"] == "correct"
+
     def test_stricter_all_than_expected_is_correct(self, maildomain_factory):
         """A domain hardening ~all into -all is stricter, not insecure."""
         maildomain = maildomain_factory(name="example.com")
@@ -1573,9 +1620,10 @@ class TestSPFValidRecordsAreNotFlagged:
             result = check_single_record(maildomain, expected_record)
             assert result["status"] == "correct"
 
-    def test_transient_dns_failure_in_chain_is_not_cached(self, maildomain_factory):
-        """A timeout while walking the chain says nothing about the record, so
-        it must not be cached as a definitive failure for 10 minutes."""
+    def test_transient_dns_failure_in_chain_is_reported_as_an_error(
+        self, maildomain_factory
+    ):
+        """A timeout while walking the chain says nothing about the record."""
         maildomain = maildomain_factory(name="example.com")
         expected_record = {
             "type": "TXT",
@@ -1595,6 +1643,62 @@ class TestSPFValidRecordsAreNotFlagged:
             result = check_single_record(maildomain, expected_record)
             assert result["status"] == "error"
 
+    @override_settings(
+        MESSAGES_TECHNICAL_DOMAIN="messages.org",
+        MESSAGES_DNS_RECORDS='[{"target":"","type":"txt",'
+        '"value":"v=spf1 include:_spf.messages.org -all"}]',
+    )
+    def test_transient_dns_failure_in_chain_is_not_cached(self, maildomain_factory):
+        """That error must not be cached as a definitive failure for 10 minutes."""
+        cache.clear()
+        maildomain = maildomain_factory(name="example.com")
+
+        with (
+            patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve,
+            patch("core.services.dns.check.cache.set") as mock_cache_set,
+        ):
+
+            def resolve_side_effect(name, _record_type):
+                if name == "example.com":
+                    return _txt_answer("v=spf1 include:_spf.messages.org -all")
+                raise Timeout()
+
+            mock_resolve.side_effect = resolve_side_effect
+
+            assert check_spf_status(maildomain) is False
+            mock_cache_set.assert_not_called()
+
+    @override_settings(
+        MESSAGES_TECHNICAL_DOMAIN="messages.org",
+        MESSAGES_DNS_RECORDS='[{"target":"","type":"txt",'
+        '"value":"v=spf1 include:_spf.messages.org -all"}]',
+    )
+    def test_unrelated_transient_failure_stays_definitive(self, maildomain_factory):
+        """A timeout on someone else's include, when ours was looked up and
+        settled, is not what hid it: the answer is definitive and cacheable."""
+        cache.clear()
+        maildomain = maildomain_factory(name="example.com")
+
+        with (
+            patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve,
+            patch("core.services.dns.check.cache.set") as mock_cache_set,
+        ):
+
+            def resolve_side_effect(name, _record_type):
+                if name == "example.com":
+                    return _txt_answer(
+                        "v=spf1 include:_spf.messages.org"
+                        " include:other.example.net -all"
+                    )
+                if name == "_spf.messages.org":
+                    raise NXDOMAIN()
+                raise Timeout()
+
+            mock_resolve.side_effect = resolve_side_effect
+
+            assert check_spf_status(maildomain) is False
+            mock_cache_set.assert_called_once()
+
 
 @pytest.fixture(name="maildomain_factory")
 def fixture_maildomain_factory():
@@ -1851,6 +1955,64 @@ class TestSPFRecursiveCheck:
 
             assert result["status"] == "duplicate"
 
+    def test_spf_weak_all_does_not_excuse_a_missing_include(
+        self, maildomain_factory, settings
+    ):
+        """A weak "all" says the policy is lax; it says nothing about whether
+        the domain delegates to us. Reporting "insecure" here would let
+        check_spf_status send for a domain that never included us."""
+        settings.MESSAGES_TECHNICAL_DOMAIN = "messages.org"
+        maildomain = maildomain_factory(name="example.com")
+        expected_record = {
+            "type": "TXT",
+            "target": "",
+            "value": "v=spf1 include:_spf.messages.org -all",
+        }
+
+        with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve:
+
+            def resolve_side_effect(name, _record_type):
+                if name == "example.com":
+                    return _txt_answer("v=spf1 ?all include:other.example.net")
+                if name == "other.example.net":
+                    return _txt_answer("v=spf1 ip4:1.2.3.4 -all")
+                raise NXDOMAIN()
+
+            mock_resolve.side_effect = resolve_side_effect
+            result = check_single_record(maildomain, expected_record)
+
+            assert result["status"] == "incorrect"
+
+    def test_spf_redirect_is_ignored_when_all_is_present(
+        self, maildomain_factory, settings
+    ):
+        """RFC 7208 5.1 and 6.1: a "redirect=" modifier MUST be ignored when the
+        record has an "all" mechanism anywhere, so it delegates nothing and our
+        include is not actually in place."""
+        settings.MESSAGES_TECHNICAL_DOMAIN = "messages.org"
+        maildomain = maildomain_factory(name="example.com")
+        expected_record = {
+            "type": "TXT",
+            "target": "",
+            "value": "v=spf1 include:_spf.messages.org -all",
+        }
+
+        with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve:
+
+            def resolve_side_effect(name, _record_type):
+                if name == "example.com":
+                    return _txt_answer("v=spf1 redirect=policy.example.net -all")
+                if name == "policy.example.net":
+                    return _txt_answer("v=spf1 include:_spf.messages.org -all")
+                if name == "_spf.messages.org":
+                    return _txt_answer("v=spf1 ip4:1.2.3.4 -all")
+                raise NXDOMAIN()
+
+            mock_resolve.side_effect = resolve_side_effect
+            result = check_single_record(maildomain, expected_record)
+
+            assert result["status"] == "incorrect"
+
 
 @pytest.mark.django_db
 class TestCheckSPFStatus: