From 4e106818b04cb7e62342e8bca8c200675c9917c0 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 11:40:21 +0000 Subject: [PATCH] Handle more sorts of TXT record values --- octodns/processor/spf.py | 37 ++++++++-- tests/test_octodns_processor_spf.py | 109 ++++++++++++++++++++++++++-- 2 files changed, 132 insertions(+), 14 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 947d651..2d3b4a2 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -2,7 +2,9 @@ # # +import re from logging import getLogger +from typing import Optional import dns.resolver @@ -26,21 +28,42 @@ class SpfDnsLookupProcessor(BaseProcessor): self.log.debug(f"SpfDnsLookupProcessor: {name}") super().__init__(name) - def _lookup( - self, record: Record, values: list[str], lookups: int = 0 - ) -> int: + def _get_spf_from_txt_values( + self, values: list[str], record: Record + ) -> Optional[str]: + self.log.debug( + f"_get_spf_from_txt_values: record={record.fqdn} values={values}" + ) + # SPF values must begin with 'v=spf1 ' spf = [value for value in values if value.startswith('v=spf1 ')] if len(spf) == 0: - return lookups + return None if len(spf) > 1: raise SpfValueException( f"{record.fqdn} has more than one SPF value" ) - spf = spf[0] + match = re.search(r"(v=spf1\s.+(?:all|redirect=))", "".join(values)) + + if match is None: + raise SpfValueException(f"{record.fqdn} has an invalid SPF value") + + return match.group() + + def _check_dns_lookups( + self, record: Record, values: list[str], lookups: int = 0 + ) -> int: + self.log.debug( + f"_check_dns_lookups: record={record.fqdn} values={values} lookups={lookups}" + ) + + spf = self._get_spf_from_txt_values(values, record) + + if spf is None: + return lookups terms = spf.removeprefix('v=spf1 ').split(' ') @@ -61,7 +84,7 @@ class SpfDnsLookupProcessor(BaseProcessor): answer = dns.resolver.resolve( term.removeprefix('include:'), 'TXT' ) - lookups = self._lookup( + lookups = self._check_dns_lookups( record, [value.to_text()[1:-1] for value in answer], lookups ) @@ -75,6 +98,6 @@ class SpfDnsLookupProcessor(BaseProcessor): if record._octodns.get('lenient'): continue - self._lookup(record, record.values) + self._check_dns_lookups(record, record.values) return zone diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index 2388242..245274c 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -10,9 +10,83 @@ from octodns.zone import Zone class TestSpfDnsLookupProcessor(TestCase): + def test_get_spf_from_txt_values(self): + processor = SpfDnsLookupProcessor('test') + record = Record.new( + Zone('unit.tests.', []), + '', + {'type': 'TXT', 'ttl': 86400, 'values': ['v=DMARC1\; p=reject\;']}, + ) + + self.assertEqual( + 'v=spf1 include:_spf.google.com ~all', + processor._get_spf_from_txt_values( + [ + 'v=DMARC1\; p=reject\;', + 'v=spf1 include:_spf.google.com ~all', + ], + record, + ), + ) + + with self.assertRaises(SpfValueException): + processor._get_spf_from_txt_values( + [ + 'v=spf1 include:_spf.google.com ~all', + 'v=spf1 include:_spf.google.com ~all', + ], + record, + ) + + self.assertEqual( + 'v=spf1 include:_spf.google.com ~all', + processor._get_spf_from_txt_values( + [ + 'v=DMARC1\; p=reject\;', + 'v=spf1 include:_spf.google.com ~all', + ], + record, + ), + ) + + with self.assertRaises(SpfValueException): + processor._get_spf_from_txt_values( + ['v=spf1 include:_spf.google.com'], record + ) + + self.assertIsNone( + processor._get_spf_from_txt_values( + ['v=DMARC1\; p=reject\;'], record + ) + ) + + # SPF record split across multiple character-strings, https://www.rfc-editor.org/rfc/rfc7208#section-3.3 + self.assertEqual( + 'v=spf1 include:_spf.google.com ip4:1.2.3.4 ~all', + processor._get_spf_from_txt_values( + [ + 'v=spf1 include:_spf.google.com', + ' ip4:1.2.3.4 ~all', + 'v=DMARC1\; p=reject\;', + ], + record, + ), + ) + + self.assertEqual( + 'v=spf1 +mx redirect=', + processor._get_spf_from_txt_values( + [ + 'v=spf1 +mx redirect=_spf.example.com', + 'v=DMARC1\; p=reject\;', + ], + record, + ), + ) + def test_processor(self): processor = SpfDnsLookupProcessor('test') - assert processor.name == 'test' + self.assertEqual('test', processor.name) processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -31,7 +105,8 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) - assert zone == processor.process_source_zone(zone) + self.assertEqual(zone, processor.process_source_zone(zone)) + zone = Zone('unit.tests.', []) zone.add_record( Record.new( @@ -48,7 +123,7 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) - assert zone == processor.process_source_zone(zone) + self.assertEqual(zone, processor.process_source_zone(zone)) zone = Zone('unit.tests.', []) zone.add_record( @@ -88,6 +163,28 @@ class TestSpfDnsLookupProcessor(TestCase): with self.assertRaises(SpfDnsLookupException): processor.process_source_zone(zone) + def test_processor_with_long_txt_values(self): + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'value': ( + '"v=spf1 ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334 ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334"' + ' " ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334 ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334"' + ' " ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334 ~all"' + ), + }, + ) + ) + + self.assertEqual(zone, processor.process_source_zone(zone)) + def test_processor_skips_lenient_records(self): processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -104,9 +201,7 @@ class TestSpfDnsLookupProcessor(TestCase): ) zone.add_record(lenient) - processed = processor.process_source_zone(zone) - - assert zone == processed + self.assertEqual(zone, processor.process_source_zone(zone)) def test_processor_errors_on_many_spf_values_in_record(self): processor = SpfDnsLookupProcessor('test') @@ -172,4 +267,4 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) - assert zone == processor.process_source_zone(zone) + self.assertEqual(zone, processor.process_source_zone(zone))