From 8b80035c15eeaa67f987ba056277f052d98e629e Mon Sep 17 00:00:00 2001 From: Sylvain Zimmer Date: Thu, 20 Aug 2026 12:18:51 +0200 Subject: [PATCH] more review fix --- src/backend/core/services/dns/check.py | 58 ++++++++----------- src/backend/core/tests/dns/test_check.py | 71 +++++++++++++++++++----- 2 files changed, 80 insertions(+), 49 deletions(-) diff --git a/src/backend/core/services/dns/check.py b/src/backend/core/services/dns/check.py index 9c36bd4b..fbc6450a 100644 --- a/src/backend/core/services/dns/check.py +++ b/src/backend/core/services/dns/check.py @@ -42,13 +42,8 @@ SPF_TERM_NAME_RE = re.compile(r"[^:=/]*") # 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( |$)|DMARC1\b)", re.IGNORECASE) +# RFC 7208 4.7: a record with no "all" ends in an implicit "?all". +SPF_IMPLICIT_ALL = "?all" def normalize_txt_value(value: str) -> str: @@ -244,13 +239,16 @@ def _check_spf(expected_value: str, found_values: List[str]) -> Dict[str, any]: def _all_is_acceptable(expected_all: Optional[str], found_all: Optional[str]) -> bool: """Whether a found "all" mechanism is at least as strict as expected. - "~all" also passes for an expected "-all": softfail is where most domains - start, and it does not keep us from sending. + A record with no "all" is read as the implicit "?all" it ends in, on + either side: without it, an expected value carrying no "all" of its own + could never be matched. "~all" also passes for an expected "-all": + softfail is where most domains start, and it does not keep us from + sending. """ + expected_all = expected_all or SPF_IMPLICIT_ALL + found_all = found_all or SPF_IMPLICIT_ALL if expected_all == found_all: return True - if expected_all is None or found_all is None: - return False if expected_all == "-all" and found_all == "~all": return True return SPF_ALL_STRICTNESS[found_all] > SPF_ALL_STRICTNESS[expected_all] @@ -334,8 +332,7 @@ def _resolve_spf_includes( answers = dns.resolver.resolve(include_domain, "TXT") spf_records = [ value - for rr in answers.rrset - for value in _txt_record_values(rr) + for value in (_txt_record_value(rr) for rr in answers.rrset) if is_spf_record(value) ] except (dns.resolver.NXDOMAIN, dns.resolver.NoAnswer): @@ -368,22 +365,20 @@ def _resolve_spf_includes( return resolved, visited, transient, None -def _txt_record_values(rr) -> List[str]: - """Normalized TXT values carried by a single resource record. +def _txt_record_value(rr) -> str: + """Normalized value of a single TXT resource record. - 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. 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. + RFC 7208 3.3 and RFC 6376 3.6.2: the strings of one record are + concatenated with no separator, which is how a value over the 255-octet + limit on a character-string is published. Separate TXT records arrive as + separate resource records, so several strings here always belong to the + same record. SPF records are US-ASCII (RFC 7208 3.1), but an unrelated + TXT record may hold anything, and that is no reason to fail the check. """ - 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))] + return normalize_txt_value(b"".join(rr.strings).decode(errors="replace")) -def _resolve_dns_values(record_type, target, query_name): +def _resolve_dns_values(record_type, query_name): """Resolve DNS and return found values and normalized expected value flag.""" if record_type.upper() == "MX": answers = dns.resolver.resolve(query_name, "MX") @@ -391,16 +386,7 @@ def _resolve_dns_values(record_type, target, query_name): if record_type.upper() == "TXT": answers = dns.resolver.resolve(query_name, "TXT") - values = [] - 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(errors="replace")) - ) - else: - values.extend(_txt_record_values(rr)) - return values + return [_txt_record_value(rr) for rr in answers.rrset] answers = dns.resolver.resolve(query_name, record_type) return [answer.to_text() for answer in answers] @@ -449,7 +435,7 @@ def check_single_record( query_name = f"{target}.{maildomain.name}" if target else maildomain.name try: - found_values = _resolve_dns_values(record_type, target, query_name) + found_values = _resolve_dns_values(record_type, query_name) if record_type.upper() == "TXT": expected_value = normalize_txt_value(expected_value) diff --git a/src/backend/core/tests/dns/test_check.py b/src/backend/core/tests/dns/test_check.py index d3ef5ef9..651efd2d 100644 --- a/src/backend/core/tests/dns/test_check.py +++ b/src/backend/core/tests/dns/test_check.py @@ -407,12 +407,16 @@ class TestDNSChecking: # pylint: disable=too-many-public-methods assert result["status"] == "correct" - def test_check_single_record_spf_found_when_resolver_merges_txt_records( + def test_check_single_record_spf_found_among_other_txt_records( self, maildomain_factory ): - """Regression: some local resolvers (e.g. systemd-resolved) merge - separate TXT records into a single RR with multiple strings. SPF must - still be found by iterating individual strings.""" + """SPF must be found when the name carries other TXT records too. + + Separate TXT records arrive as separate resource records, each with + its own strings: the RDATA boundary is part of the record framing, so + no resolver can merge two records into one. Several strings on one + record therefore always belong together (RFC 7208 3.3). + """ maildomain = maildomain_factory(name="example.com") expected_record = { "type": "TXT", @@ -421,15 +425,10 @@ class TestDNSChecking: # pylint: disable=too-many-public-methods } with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve: - # Single RR with two strings (merged by local resolver) - merged_rr = MagicMock() - merged_rr.strings = ( - b"google-site-verification=abc123", - b"v=spf1 include:_spf.example.com -all", + mock_resolve.return_value = _txt_answer( + "google-site-verification=abc123", + "v=spf1 include:_spf.example.com -all", ) - answer = MagicMock() - answer.rrset = [merged_rr] - mock_resolve.return_value = answer result = check_single_record(maildomain, expected_record) assert result["status"] == "correct" @@ -1422,7 +1421,9 @@ class TestSPFValidRecordsAreNotFlagged: def test_record_split_across_several_strings(self, maildomain_factory): """RFC 7208 3.3: the strings of one TXT record are concatenated with no - separator, which is how records over 255 octets are published.""" + separator, which is how a value over the 255-octet character-string + limit is published. Publishers split where they like, not only at + exactly 255 octets, so the split point carries no meaning.""" maildomain = maildomain_factory(name="example.com") expected_record = { "type": "TXT", @@ -1555,6 +1556,50 @@ class TestSPFValidRecordsAreNotFlagged: result = check_single_record(maildomain, expected_record) assert result["status"] == "correct" + def test_expected_value_without_an_all_mechanism(self, maildomain_factory): + """A configured expected value carrying no "all" ends in the implicit + "?all" (RFC 7208 4.7); a stricter published one satisfies it. Reading + it as "no acceptable all exists" would make the check unsatisfiable.""" + maildomain = maildomain_factory(name="example.com") + expected_record = { + "type": "TXT", + "target": "", + "value": "v=spf1 include:_spf.example.com", + } + + with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve: + mock_resolve.side_effect = self._resolver( + { + "example.com": _txt_answer("v=spf1 include:_spf.example.com -all"), + "_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_expected_value_without_an_all_still_rejects_plus_all( + self, maildomain_factory + ): + """The implicit "?all" is still stricter than a published "+all".""" + maildomain = maildomain_factory(name="example.com") + expected_record = { + "type": "TXT", + "target": "", + "value": "v=spf1 include:_spf.example.com", + } + + with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve: + mock_resolve.side_effect = self._resolver( + { + "example.com": _txt_answer("v=spf1 include:_spf.example.com +all"), + "_spf.example.com": _txt_answer("v=spf1 ip4:1.2.3.4 -all"), + } + ) + + result = check_single_record(maildomain, expected_record) + assert result["status"] == "insecure" + 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