From 9f4a910a0809df194165aacda611f70b6714789b Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Wed, 15 Feb 2023 13:30:18 +0000 Subject: [PATCH 01/13] Upgrade `dnspython` Matches the minimum version required by `octodns-bind`. --- requirements.txt | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index 7371f9c..cb174a4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,5 @@ PyYAML==6.0 -dnspython==2.2.1 +dnspython==2.3.0 fqdn==1.5.1 idna==3.4 natsort==8.2.0 diff --git a/setup.py b/setup.py index e6d97e5..3b80b46 100644 --- a/setup.py +++ b/setup.py @@ -84,7 +84,7 @@ setup( }, install_requires=( 'PyYaml>=4.2b1', - 'dnspython>=1.15.0', + 'dnspython>=2.2.1', 'fqdn>=1.5.0', 'idna>=3.3', 'natsort>=5.5.0', From dc446eefb99f46a75c8f767f87739d692b929351 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Wed, 15 Feb 2023 15:17:12 +0000 Subject: [PATCH 02/13] Error on too many lookups from single SPF mechanisms --- octodns/processor/spf.py | 58 +++++++++++ tests/test_octodns_processor_spf.py | 153 ++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+) create mode 100644 octodns/processor/spf.py create mode 100644 tests/test_octodns_processor_spf.py diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py new file mode 100644 index 0000000..258ad76 --- /dev/null +++ b/octodns/processor/spf.py @@ -0,0 +1,58 @@ +# +# +# + +from logging import getLogger + +from .base import BaseProcessor, ProcessorException + + +class SpfValueException(ProcessorException): + pass + + +class SpfDnsLookupException(ProcessorException): + pass + + +class SpfDnsLookupProcessor(BaseProcessor): + log = getLogger('SpfDnsLookupProcessor') + + def __init__(self, name): + self.log.debug(f"SpfDnsLookupProcessor: {name}") + super().__init__(name) + + def process_source_zone(self, zone, *args, **kwargs): + for record in zone.records: + if record._type != 'TXT': + continue + + if record._octodns.get('lenient'): + continue + + # SPF values must begin with 'v=spf1 ' + values = [ + value for value in record.values if value.startswith('v=spf1 ') + ] + + if len(values) == 0: + continue + + if len(values) > 1: + raise SpfValueException( + f"{record.fqdn} has more than one SPF value" + ) + + lookups = 0 + terms = values[0].removeprefix('v=spf1 ').split(' ') + + for term in terms: + if lookups > 10: + raise SpfDnsLookupException( + f"{record.fqdn} has too many SPF DNS lookups" + ) + + if term in ['a', 'mx', 'exists', 'redirect']: + lookups += 1 + + return zone diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py new file mode 100644 index 0000000..b4cd3a1 --- /dev/null +++ b/tests/test_octodns_processor_spf.py @@ -0,0 +1,153 @@ +from unittest import TestCase + +from octodns.processor.spf import ( + SpfDnsLookupException, + SpfDnsLookupProcessor, + SpfValueException, +) +from octodns.record.base import Record +from octodns.zone import Zone + + +class TestSpfDnsLookupProcessor(TestCase): + def test_processor(self): + processor = SpfDnsLookupProcessor('test') + assert processor.name == 'test' + + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': ['v=spf1 a ~all', 'v=DMARC1\; p=reject\;'], + }, + ) + ) + + assert zone == processor.process_source_zone(zone) + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 a a a a a a a a a a -all', + 'v=DMARC1\; p=reject\;', + ], + }, + ) + ) + + assert zone == processor.process_source_zone(zone) + + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 a mx exists redirect a a a a a a a ~all', + 'v=DMARC1\; p=reject\;', + ], + }, + ) + ) + + with self.assertRaises(SpfDnsLookupException): + processor.process_source_zone(zone) + + def test_processor_skips_lenient_records(self): + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + + lenient = Record.new( + zone, + 'lenient', + { + 'type': 'TXT', + 'ttl': 86400, + 'value': 'v=spf1 a a a a a a a a a a a ~all', + 'octodns': {'lenient': True}, + }, + ) + zone.add_record(lenient) + + processed = processor.process_source_zone(zone) + + assert zone == processed + + def test_processor_errors_on_many_spf_values_in_record(self): + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + + record = Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 include:mailgun.org ~all', + 'v=spf1 include:_spf.google.com ~all', + ], + }, + ) + zone.add_record(record) + + with self.assertRaises(SpfValueException): + processor.process_source_zone(zone) + + def test_processor_filters_to_records_with_spf_values(self): + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, '', {'type': 'A', 'ttl': 86400, 'value': '1.2.3.4'} + ) + ) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'value': 'v=spf1 a a a a a a a a a a a ~all', + }, + ) + ) + + with self.assertRaises(SpfDnsLookupException): + processor.process_source_zone(zone) + + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, '', {'type': 'A', 'ttl': 86400, 'value': '1.2.3.4'} + ) + ) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': ['AAAAAAAAAAA', 'v=spf10'], + }, + ) + ) + + assert zone == processor.process_source_zone(zone) From dfc0760adf8a966c11e8f87ff8e10faf87787065 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Wed, 15 Feb 2023 16:45:50 +0000 Subject: [PATCH 03/13] Count extra lookups for the `include` mechanism Co-authored-by: Jon Nangle --- octodns/processor/spf.py | 70 +++++++++++++++++++---------- tests/test_octodns_processor_spf.py | 28 ++++++++++-- 2 files changed, 71 insertions(+), 27 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 258ad76..947d651 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -4,6 +4,10 @@ from logging import getLogger +import dns.resolver + +from octodns.record.base import Record + from .base import BaseProcessor, ProcessorException @@ -22,6 +26,47 @@ class SpfDnsLookupProcessor(BaseProcessor): self.log.debug(f"SpfDnsLookupProcessor: {name}") super().__init__(name) + def _lookup( + self, record: Record, values: list[str], lookups: int = 0 + ) -> int: + # SPF values must begin with 'v=spf1 ' + spf = [value for value in values if value.startswith('v=spf1 ')] + + if len(spf) == 0: + return lookups + + if len(spf) > 1: + raise SpfValueException( + f"{record.fqdn} has more than one SPF value" + ) + + spf = spf[0] + + terms = spf.removeprefix('v=spf1 ').split(' ') + + for term in terms: + if lookups > 10: + raise SpfDnsLookupException( + f"{record.fqdn} exceeds the 10 DNS lookup limit in the SPF record" + ) + + # These mechanisms cost one DNS lookup each + if term.startswith( + ('a', 'mx', 'exists:', 'redirect', 'include:', 'ptr') + ): + lookups += 1 + + # The include mechanism can result in further lookups after resolving the DNS record + if term.startswith('include:'): + answer = dns.resolver.resolve( + term.removeprefix('include:'), 'TXT' + ) + lookups = self._lookup( + record, [value.to_text()[1:-1] for value in answer], lookups + ) + + return lookups + def process_source_zone(self, zone, *args, **kwargs): for record in zone.records: if record._type != 'TXT': @@ -30,29 +75,6 @@ class SpfDnsLookupProcessor(BaseProcessor): if record._octodns.get('lenient'): continue - # SPF values must begin with 'v=spf1 ' - values = [ - value for value in record.values if value.startswith('v=spf1 ') - ] - - if len(values) == 0: - continue - - if len(values) > 1: - raise SpfValueException( - f"{record.fqdn} has more than one SPF value" - ) - - lookups = 0 - terms = values[0].removeprefix('v=spf1 ').split(' ') - - for term in terms: - if lookups > 10: - raise SpfDnsLookupException( - f"{record.fqdn} has too many SPF DNS lookups" - ) - - if term in ['a', 'mx', 'exists', 'redirect']: - lookups += 1 + self._lookup(record, record.values) return zone diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index b4cd3a1..2388242 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -23,7 +23,10 @@ class TestSpfDnsLookupProcessor(TestCase): { 'type': 'TXT', 'ttl': 86400, - 'values': ['v=spf1 a ~all', 'v=DMARC1\; p=reject\;'], + 'values': [ + 'v=spf1 a include:_spf.google.com ~all', + 'v=DMARC1\; p=reject\;', + ], }, ) ) @@ -38,7 +41,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'type': 'TXT', 'ttl': 86400, 'values': [ - 'v=spf1 a a a a a a a a a a -all', + 'v=spf1 a ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 -all', 'v=DMARC1\; p=reject\;', ], }, @@ -56,7 +59,26 @@ class TestSpfDnsLookupProcessor(TestCase): 'type': 'TXT', 'ttl': 86400, 'values': [ - 'v=spf1 a mx exists redirect a a a a a a a ~all', + 'v=spf1 a mx exists:example.com a a a a a a a a ~all', + 'v=DMARC1\; p=reject\;', + ], + }, + ) + ) + + with self.assertRaises(SpfDnsLookupException): + processor.process_source_zone(zone) + + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 include:example.com include:_spf.google.com include:_spf.google.com include:_spf.google.com ~all', 'v=DMARC1\; p=reject\;', ], }, From 4e106818b04cb7e62342e8bca8c200675c9917c0 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 11:40:21 +0000 Subject: [PATCH 04/13] 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)) From ffe35b9096196a534b5b3af030a15f4f3842ebb4 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 12:05:50 +0000 Subject: [PATCH 05/13] Mock calls to `dns.resolver.resolve` --- tests/test_octodns_processor_spf.py | 127 ++++++++++++---------------- 1 file changed, 52 insertions(+), 75 deletions(-) diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index 245274c..efb59e8 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -1,4 +1,5 @@ from unittest import TestCase +from unittest.mock import MagicMock, patch from octodns.processor.spf import ( SpfDnsLookupException, @@ -19,12 +20,9 @@ class TestSpfDnsLookupProcessor(TestCase): ) self.assertEqual( - 'v=spf1 include:_spf.google.com ~all', + 'v=spf1 include:example.com ~all', processor._get_spf_from_txt_values( - [ - 'v=DMARC1\; p=reject\;', - 'v=spf1 include:_spf.google.com ~all', - ], + ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], record, ), ) @@ -32,26 +30,23 @@ class TestSpfDnsLookupProcessor(TestCase): with self.assertRaises(SpfValueException): processor._get_spf_from_txt_values( [ - 'v=spf1 include:_spf.google.com ~all', - 'v=spf1 include:_spf.google.com ~all', + 'v=spf1 include:example.com ~all', + 'v=spf1 include:example.com ~all', ], record, ) self.assertEqual( - 'v=spf1 include:_spf.google.com ~all', + 'v=spf1 include:example.com ~all', processor._get_spf_from_txt_values( - [ - 'v=DMARC1\; p=reject\;', - 'v=spf1 include:_spf.google.com ~all', - ], + ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], record, ), ) with self.assertRaises(SpfValueException): processor._get_spf_from_txt_values( - ['v=spf1 include:_spf.google.com'], record + ['v=spf1 include:example.com'], record ) self.assertIsNone( @@ -62,10 +57,10 @@ class TestSpfDnsLookupProcessor(TestCase): # 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', + 'v=spf1 include:example.com ip4:1.2.3.4 ~all', processor._get_spf_from_txt_values( [ - 'v=spf1 include:_spf.google.com', + 'v=spf1 include:example.com', ' ip4:1.2.3.4 ~all', 'v=DMARC1\; p=reject\;', ], @@ -76,19 +71,16 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual( 'v=spf1 +mx redirect=', processor._get_spf_from_txt_values( - [ - 'v=spf1 +mx redirect=_spf.example.com', - 'v=DMARC1\; p=reject\;', - ], + ['v=spf1 +mx redirect=example.com', 'v=DMARC1\; p=reject\;'], record, ), ) - def test_processor(self): + @patch('dns.resolver.resolve') + def test_processor(self, resolver_mock): processor = SpfDnsLookupProcessor('test') self.assertEqual('test', processor.name) - processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) zone.add_record( Record.new( @@ -97,13 +89,15 @@ class TestSpfDnsLookupProcessor(TestCase): { 'type': 'TXT', 'ttl': 86400, - 'values': [ - 'v=spf1 a include:_spf.google.com ~all', - 'v=DMARC1\; p=reject\;', - ], + 'values': ['v=DMARC1\; p=reject\;'], }, ) ) + zone.add_record( + Record.new( + zone, '', {'type': 'A', 'ttl': 86400, 'value': '1.2.3.4'} + ) + ) self.assertEqual(zone, processor.process_source_zone(zone)) @@ -116,7 +110,29 @@ class TestSpfDnsLookupProcessor(TestCase): 'type': 'TXT', 'ttl': 86400, 'values': [ - 'v=spf1 a ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 ip4:1.2.3.4 -all', + 'v=spf1 a include:example.com ~all', + 'v=DMARC1\; p=reject\;', + ], + }, + ) + ) + + txt_value_mock = MagicMock() + txt_value_mock.to_text.return_value = '"v=spf1 -all"' + resolver_mock.return_value = [txt_value_mock] + + self.assertEqual(zone, processor.process_source_zone(zone)) + + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 a ip4:1.2.3.4 ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334 -all', 'v=DMARC1\; p=reject\;', ], }, @@ -153,17 +169,23 @@ class TestSpfDnsLookupProcessor(TestCase): 'type': 'TXT', 'ttl': 86400, 'values': [ - 'v=spf1 include:example.com include:_spf.google.com include:_spf.google.com include:_spf.google.com ~all', + 'v=spf1 include:example.com -all', 'v=DMARC1\; p=reject\;', ], }, ) ) + txt_value_mock = MagicMock() + txt_value_mock.to_text.return_value = ( + '"v=spf1 a a a a a a a a a a a -all"' + ) + resolver_mock.return_value = [txt_value_mock] + with self.assertRaises(SpfDnsLookupException): processor.process_source_zone(zone) - def test_processor_with_long_txt_values(self): + def test_processor_with_long_txt_value(self): processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -185,7 +207,7 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual(zone, processor.process_source_zone(zone)) - def test_processor_skips_lenient_records(self): + def test_processor_with_lenient_record(self): processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -203,7 +225,7 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual(zone, processor.process_source_zone(zone)) - def test_processor_errors_on_many_spf_values_in_record(self): + def test_processor_errors_on_too_many_spf_values(self): processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -223,48 +245,3 @@ class TestSpfDnsLookupProcessor(TestCase): with self.assertRaises(SpfValueException): processor.process_source_zone(zone) - - def test_processor_filters_to_records_with_spf_values(self): - processor = SpfDnsLookupProcessor('test') - zone = Zone('unit.tests.', []) - - zone.add_record( - Record.new( - zone, '', {'type': 'A', 'ttl': 86400, 'value': '1.2.3.4'} - ) - ) - zone.add_record( - Record.new( - zone, - '', - { - 'type': 'TXT', - 'ttl': 86400, - 'value': 'v=spf1 a a a a a a a a a a a ~all', - }, - ) - ) - - with self.assertRaises(SpfDnsLookupException): - processor.process_source_zone(zone) - - zone = Zone('unit.tests.', []) - - zone.add_record( - Record.new( - zone, '', {'type': 'A', 'ttl': 86400, 'value': '1.2.3.4'} - ) - ) - zone.add_record( - Record.new( - zone, - '', - { - 'type': 'TXT', - 'ttl': 86400, - 'values': ['AAAAAAAAAAA', 'v=spf10'], - }, - ) - ) - - self.assertEqual(zone, processor.process_source_zone(zone)) From ee44779f7fdd8d489a0e73c59c964c36874c5a65 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 12:32:22 +0000 Subject: [PATCH 06/13] Error on `ptr` mechanisms It has been deprecated in https://datatracker.ietf.org/doc/html/rfc7208#section-5.5. --- octodns/processor/spf.py | 9 ++-- tests/test_octodns_processor_spf.py | 66 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 2d3b4a2..fbecc7e 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -73,10 +73,13 @@ class SpfDnsLookupProcessor(BaseProcessor): f"{record.fqdn} exceeds the 10 DNS lookup limit in the SPF record" ) + if term.startswith('ptr'): + raise SpfValueException( + f"{record.fqdn} uses the deprecated ptr mechanism" + ) + # These mechanisms cost one DNS lookup each - if term.startswith( - ('a', 'mx', 'exists:', 'redirect', 'include:', 'ptr') - ): + if term.startswith(('a', 'mx', 'exists:', 'redirect', 'include:')): lookups += 1 # The include mechanism can result in further lookups after resolving the DNS record diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index efb59e8..a9b9828 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -245,3 +245,69 @@ class TestSpfDnsLookupProcessor(TestCase): with self.assertRaises(SpfValueException): processor.process_source_zone(zone) + + @patch('dns.resolver.resolve') + def test_processor_errors_ptr_mechanisms(self, resolver_mock): + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, + '', + {'type': 'TXT', 'ttl': 86400, 'values': ['v=spf1 ptr ~all']}, + ) + ) + + with self.assertRaises(SpfValueException) as context: + processor.process_source_zone(zone) + self.assertEqual( + 'unit.tests. uses the deprecated ptr mechanism', + str(context.exception), + ) + + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': ['v=spf1 ptr:example.com ~all'], + }, + ) + ) + + with self.assertRaises(SpfValueException) as context: + processor.process_source_zone(zone) + self.assertEqual( + 'unit.tests. uses the deprecated ptr mechanism', + str(context.exception), + ) + + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': ['v=spf1 include:example.com ~all'], + }, + ) + ) + + txt_value_mock = MagicMock() + txt_value_mock.to_text.return_value = '"v=spf1 ptr -all"' + resolver_mock.return_value = [txt_value_mock] + + with self.assertRaises(SpfValueException) as context: + processor.process_source_zone(zone) + self.assertEqual( + 'unit.tests. uses the deprecated ptr mechanism', + str(context.exception), + ) From 063bf78b9f3c22d7f491292f4ee3c67b18d4bb70 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 13:43:50 +0000 Subject: [PATCH 07/13] Test nested include mechanisms --- octodns/processor/spf.py | 16 +++--- tests/test_octodns_processor_spf.py | 83 ++++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 16 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index fbecc7e..0f5b5d6 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -29,7 +29,7 @@ class SpfDnsLookupProcessor(BaseProcessor): super().__init__(name) def _get_spf_from_txt_values( - self, values: list[str], record: Record + self, record: Record, values: list[str] ) -> Optional[str]: self.log.debug( f"_get_spf_from_txt_values: record={record.fqdn} values={values}" @@ -43,7 +43,7 @@ class SpfDnsLookupProcessor(BaseProcessor): if len(spf) > 1: raise SpfValueException( - f"{record.fqdn} has more than one SPF value" + f"{record.fqdn} has more than one SPF value in the TXT record" ) match = re.search(r"(v=spf1\s.+(?:all|redirect=))", "".join(values)) @@ -60,7 +60,7 @@ class SpfDnsLookupProcessor(BaseProcessor): f"_check_dns_lookups: record={record.fqdn} values={values} lookups={lookups}" ) - spf = self._get_spf_from_txt_values(values, record) + spf = self._get_spf_from_txt_values(record, values) if spf is None: return lookups @@ -84,11 +84,11 @@ class SpfDnsLookupProcessor(BaseProcessor): # The include mechanism can result in further lookups after resolving the DNS record if term.startswith('include:'): - answer = dns.resolver.resolve( - term.removeprefix('include:'), 'TXT' - ) + domain = term.removeprefix('include:') + answer = dns.resolver.resolve(domain, 'TXT') + answer_values = [value.to_text()[1:-1] for value in answer] lookups = self._check_dns_lookups( - record, [value.to_text()[1:-1] for value in answer], lookups + record, answer_values, lookups ) return lookups @@ -101,6 +101,6 @@ class SpfDnsLookupProcessor(BaseProcessor): if record._octodns.get('lenient'): continue - self._check_dns_lookups(record, record.values) + self._check_dns_lookups(record, record.values, 0) return zone diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index a9b9828..6784b9d 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -1,5 +1,5 @@ from unittest import TestCase -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch from octodns.processor.spf import ( SpfDnsLookupException, @@ -22,36 +22,36 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual( 'v=spf1 include:example.com ~all', processor._get_spf_from_txt_values( - ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], record, + ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], ), ) with self.assertRaises(SpfValueException): processor._get_spf_from_txt_values( + record, [ 'v=spf1 include:example.com ~all', 'v=spf1 include:example.com ~all', ], - record, ) self.assertEqual( 'v=spf1 include:example.com ~all', processor._get_spf_from_txt_values( - ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], record, + ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], ), ) with self.assertRaises(SpfValueException): processor._get_spf_from_txt_values( - ['v=spf1 include:example.com'], record + record, ['v=spf1 include:example.com'] ) self.assertIsNone( processor._get_spf_from_txt_values( - ['v=DMARC1\; p=reject\;'], record + record, ['v=DMARC1\; p=reject\;'] ) ) @@ -59,20 +59,20 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual( 'v=spf1 include:example.com ip4:1.2.3.4 ~all', processor._get_spf_from_txt_values( + record, [ 'v=spf1 include:example.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=example.com', 'v=DMARC1\; p=reject\;'], record, + ['v=spf1 +mx redirect=example.com', 'v=DMARC1\; p=reject\;'], ), ) @@ -117,11 +117,13 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) + resolver_mock.reset_mock() txt_value_mock = MagicMock() txt_value_mock.to_text.return_value = '"v=spf1 -all"' resolver_mock.return_value = [txt_value_mock] self.assertEqual(zone, processor.process_source_zone(zone)) + resolver_mock.assert_called_once_with('example.com', 'TXT') zone = Zone('unit.tests.', []) zone.add_record( @@ -176,6 +178,7 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) + resolver_mock.reset_mock() txt_value_mock = MagicMock() txt_value_mock.to_text.return_value = ( '"v=spf1 a a a a a a a a a a a -all"' @@ -184,6 +187,40 @@ class TestSpfDnsLookupProcessor(TestCase): with self.assertRaises(SpfDnsLookupException): processor.process_source_zone(zone) + resolver_mock.assert_called_once_with('example.com', 'TXT') + + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 include:example.com -all', + 'v=DMARC1\; p=reject\;', + ], + }, + ) + ) + + resolver_mock.reset_mock() + first_txt_value_mock = MagicMock() + first_txt_value_mock.to_text.return_value = ( + '"v=spf1 include:_spf.example.com -all"' + ) + second_txt_value_mock = MagicMock() + second_txt_value_mock.to_text.return_value = '"v=spf1 a -all"' + resolver_mock.side_effect = [ + [first_txt_value_mock], + [second_txt_value_mock], + ] + + self.assertEqual(zone, processor.process_source_zone(zone)) + resolver_mock.assert_has_calls( + [call('example.com', 'TXT'), call('_spf.example.com', 'TXT')] + ) def test_processor_with_long_txt_value(self): processor = SpfDnsLookupProcessor('test') @@ -311,3 +348,33 @@ class TestSpfDnsLookupProcessor(TestCase): 'unit.tests. uses the deprecated ptr mechanism', str(context.exception), ) + resolver_mock.assert_called_once_with('example.com', 'TXT') + + @patch('dns.resolver.resolve') + def test_processor_errors_on_recursive_include_mechanism( + self, resolver_mock + ): + processor = SpfDnsLookupProcessor('test') + zone = Zone('unit.tests.', []) + + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': ['v=spf1 include:example.com ~all'], + }, + ) + ) + + txt_value_mock = MagicMock() + txt_value_mock.to_text.return_value = ( + '"v=spf1 include:example.com ~all"' + ) + resolver_mock.return_value = [txt_value_mock] + + with self.assertRaises(SpfDnsLookupException): + processor.process_source_zone(zone) + resolver_mock.assert_called_with('example.com', 'TXT') From ae0497bb5ad9778685ac1ecc27686cfe99d0e8b5 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 13:48:47 +0000 Subject: [PATCH 08/13] Add class comment --- octodns/processor/spf.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 0f5b5d6..40ffa72 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -22,6 +22,35 @@ class SpfDnsLookupException(ProcessorException): class SpfDnsLookupProcessor(BaseProcessor): + ''' + Validate that SPF values in TXT records are valid. + + Example usage: + + processors: + spf: + class: octodns.processor.spf.SpfDnsLookupProcessor + + zones: + example.com.: + sources: + - config + processors: + - spf + targets: + - route53 + + The validation can be skipped for specific records by setting the lenient + flag, e.g. + + _spf: + octodns: + lenient: true + ttl: 86400 + type: TXT + value: v=spf1 ptr ~all + ''' + log = getLogger('SpfDnsLookupProcessor') def __init__(self, name): From 19e8a271109328e8543ff9396f5ecf86c86ffcf3 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 16:12:17 +0000 Subject: [PATCH 09/13] Handle chunked values from DNS lookups --- octodns/processor/spf.py | 21 ++++--- tests/test_octodns_processor_spf.py | 90 ++++++++++++++++------------- 2 files changed, 64 insertions(+), 47 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 40ffa72..35a395a 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -2,11 +2,11 @@ # # -import re from logging import getLogger from typing import Optional import dns.resolver +from dns.resolver import Answer from octodns.record.base import Record @@ -64,23 +64,30 @@ class SpfDnsLookupProcessor(BaseProcessor): f"_get_spf_from_txt_values: record={record.fqdn} values={values}" ) - # SPF values must begin with 'v=spf1 ' + # SPF values to validate will begin with 'v=spf1 ' spf = [value for value in values if value.startswith('v=spf1 ')] + # No SPF values in the TXT record if len(spf) == 0: return None + # More than one SPF value resolves as "permerror", https://datatracker.ietf.org/doc/html/rfc7208#section-4.5 if len(spf) > 1: raise SpfValueException( f"{record.fqdn} has more than one SPF value in the TXT record" ) - match = re.search(r"(v=spf1\s.+(?:all|redirect=))", "".join(values)) + return spf[0] - if match is None: - raise SpfValueException(f"{record.fqdn} has an invalid SPF value") + def _process_answer(self, answer: Answer) -> list[str]: + values = [] - return match.group() + for value in answer: + text_value = value.to_text() + processed_value = text_value[1:-1].replace('" "', '') + values.append(processed_value) + + return values def _check_dns_lookups( self, record: Record, values: list[str], lookups: int = 0 @@ -115,7 +122,7 @@ class SpfDnsLookupProcessor(BaseProcessor): if term.startswith('include:'): domain = term.removeprefix('include:') answer = dns.resolver.resolve(domain, 'TXT') - answer_values = [value.to_text()[1:-1] for value in answer] + answer_values = self._process_answer(answer) lookups = self._check_dns_lookups( record, answer_values, lookups ) diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index 6784b9d..e895f92 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -13,63 +13,47 @@ from octodns.zone import Zone class TestSpfDnsLookupProcessor(TestCase): def test_get_spf_from_txt_values(self): processor = SpfDnsLookupProcessor('test') + + # Used in logging record = Record.new( Zone('unit.tests.', []), '', - {'type': 'TXT', 'ttl': 86400, 'values': ['v=DMARC1\; p=reject\;']}, + {'type': 'TXT', 'ttl': 86400, 'values': ['']}, ) - self.assertEqual( - 'v=spf1 include:example.com ~all', - processor._get_spf_from_txt_values( - record, - ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], - ), - ) - - with self.assertRaises(SpfValueException): - processor._get_spf_from_txt_values( - record, - [ - 'v=spf1 include:example.com ~all', - 'v=spf1 include:example.com ~all', - ], - ) - - self.assertEqual( - 'v=spf1 include:example.com ~all', - processor._get_spf_from_txt_values( - record, - ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], - ), - ) - - with self.assertRaises(SpfValueException): - processor._get_spf_from_txt_values( - record, ['v=spf1 include:example.com'] - ) - self.assertIsNone( processor._get_spf_from_txt_values( record, ['v=DMARC1\; p=reject\;'] ) ) - # SPF record split across multiple character-strings, https://www.rfc-editor.org/rfc/rfc7208#section-3.3 self.assertEqual( - 'v=spf1 include:example.com ip4:1.2.3.4 ~all', + 'v=spf1 include:example.com ~all', + processor._get_spf_from_txt_values( + record, + ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], + ), + ) + + with self.assertRaises(SpfValueException): processor._get_spf_from_txt_values( record, [ - 'v=spf1 include:example.com', - ' ip4:1.2.3.4 ~all', - 'v=DMARC1\; p=reject\;', + 'v=spf1 include:example.com ~all', + 'v=spf1 include:example.com ~all', ], + ) + + # Missing "all" or "redirect" at the end + self.assertEqual( + 'v=spf1 include:example.com', + processor._get_spf_from_txt_values( + record, ['v=spf1 include:example.com', 'v=DMARC1\; p=reject\;'] ), ) self.assertEqual( - 'v=spf1 +mx redirect=', + 'v=spf1 +mx redirect=example.com', processor._get_spf_from_txt_values( record, ['v=spf1 +mx redirect=example.com', 'v=DMARC1\; p=reject\;'], @@ -117,7 +101,7 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) - resolver_mock.reset_mock() + resolver_mock.reset_mock(return_value=True, side_effect=True) txt_value_mock = MagicMock() txt_value_mock.to_text.return_value = '"v=spf1 -all"' resolver_mock.return_value = [txt_value_mock] @@ -178,7 +162,7 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) - resolver_mock.reset_mock() + resolver_mock.reset_mock(return_value=True, side_effect=True) txt_value_mock = MagicMock() txt_value_mock.to_text.return_value = ( '"v=spf1 a a a a a a a a a a a -all"' @@ -205,7 +189,33 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) - resolver_mock.reset_mock() + resolver_mock.reset_mock(return_value=True, side_effect=True) + txt_value_mock = MagicMock() + txt_value_mock.to_text.return_value = ( + '"v=spf1 ip4:1.2.3.4" " ip4:4.3.2.1 -all"' + ) + resolver_mock.return_value = [txt_value_mock] + + self.assertEqual(zone, processor.process_source_zone(zone)) + resolver_mock.assert_called_once_with('example.com', 'TXT') + + zone = Zone('unit.tests.', []) + zone.add_record( + Record.new( + zone, + '', + { + 'type': 'TXT', + 'ttl': 86400, + 'values': [ + 'v=spf1 include:example.com -all', + 'v=DMARC1\; p=reject\;', + ], + }, + ) + ) + + resolver_mock.reset_mock(return_value=True, side_effect=True) first_txt_value_mock = MagicMock() first_txt_value_mock.to_text.return_value = ( '"v=spf1 include:_spf.example.com -all"' From 70fe46f76b3496b8b09e3182de0e3fe7b4c4a22c Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Thu, 16 Feb 2023 16:23:15 +0000 Subject: [PATCH 10/13] Patch `dns.resolver.resolve` in all processor tests --- tests/test_octodns_processor_spf.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index e895f92..01beebb 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -244,9 +244,13 @@ class TestSpfDnsLookupProcessor(TestCase): '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"' + '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 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' + ' 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' ), }, ) @@ -254,7 +258,8 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual(zone, processor.process_source_zone(zone)) - def test_processor_with_lenient_record(self): + @patch('dns.resolver.resolve') + def test_processor_with_lenient_record(self, resolver_mock): processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -271,8 +276,10 @@ class TestSpfDnsLookupProcessor(TestCase): zone.add_record(lenient) self.assertEqual(zone, processor.process_source_zone(zone)) + resolver_mock.assert_not_called() - def test_processor_errors_on_too_many_spf_values(self): + @patch('dns.resolver.resolve') + def test_processor_errors_on_too_many_spf_values(self, resolver_mock): processor = SpfDnsLookupProcessor('test') zone = Zone('unit.tests.', []) @@ -283,8 +290,8 @@ class TestSpfDnsLookupProcessor(TestCase): 'type': 'TXT', 'ttl': 86400, 'values': [ - 'v=spf1 include:mailgun.org ~all', 'v=spf1 include:_spf.google.com ~all', + 'v=spf1 include:mailgun.org ~all', ], }, ) @@ -292,6 +299,7 @@ class TestSpfDnsLookupProcessor(TestCase): with self.assertRaises(SpfValueException): processor.process_source_zone(zone) + resolver_mock.assert_not_called() @patch('dns.resolver.resolve') def test_processor_errors_ptr_mechanisms(self, resolver_mock): @@ -312,6 +320,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'unit.tests. uses the deprecated ptr mechanism', str(context.exception), ) + resolver_mock.assert_not_called() zone = Zone('unit.tests.', []) @@ -327,12 +336,15 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) + resolver_mock.reset_mock(return_value=True, side_effect=True) + with self.assertRaises(SpfValueException) as context: processor.process_source_zone(zone) self.assertEqual( 'unit.tests. uses the deprecated ptr mechanism', str(context.exception), ) + resolver_mock.assert_not_called() zone = Zone('unit.tests.', []) @@ -348,6 +360,7 @@ class TestSpfDnsLookupProcessor(TestCase): ) ) + resolver_mock.reset_mock(return_value=True, side_effect=True) txt_value_mock = MagicMock() txt_value_mock.to_text.return_value = '"v=spf1 ptr -all"' resolver_mock.return_value = [txt_value_mock] From 25206426ea5dbb9632e62f4470a429f1095b2cd0 Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Mon, 20 Feb 2023 18:13:05 +0000 Subject: [PATCH 11/13] Escape the DMARC1 values properly --- tests/test_octodns_processor_spf.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/test_octodns_processor_spf.py b/tests/test_octodns_processor_spf.py index 01beebb..a1d22ae 100644 --- a/tests/test_octodns_processor_spf.py +++ b/tests/test_octodns_processor_spf.py @@ -23,7 +23,7 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertIsNone( processor._get_spf_from_txt_values( - record, ['v=DMARC1\; p=reject\;'] + record, ['v=DMARC1\\; p=reject\\;'] ) ) @@ -31,7 +31,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'v=spf1 include:example.com ~all', processor._get_spf_from_txt_values( record, - ['v=DMARC1\; p=reject\;', 'v=spf1 include:example.com ~all'], + ['v=DMARC1\\; p=reject\\;', 'v=spf1 include:example.com ~all'], ), ) @@ -48,7 +48,8 @@ class TestSpfDnsLookupProcessor(TestCase): self.assertEqual( 'v=spf1 include:example.com', processor._get_spf_from_txt_values( - record, ['v=spf1 include:example.com', 'v=DMARC1\; p=reject\;'] + record, + ['v=spf1 include:example.com', 'v=DMARC1\\; p=reject\\;'], ), ) @@ -56,7 +57,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'v=spf1 +mx redirect=example.com', processor._get_spf_from_txt_values( record, - ['v=spf1 +mx redirect=example.com', 'v=DMARC1\; p=reject\;'], + ['v=spf1 +mx redirect=example.com', 'v=DMARC1\\; p=reject\\;'], ), ) @@ -73,7 +74,7 @@ class TestSpfDnsLookupProcessor(TestCase): { 'type': 'TXT', 'ttl': 86400, - 'values': ['v=DMARC1\; p=reject\;'], + 'values': ['v=DMARC1\\; p=reject\\;'], }, ) ) @@ -95,7 +96,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'ttl': 86400, 'values': [ 'v=spf1 a include:example.com ~all', - 'v=DMARC1\; p=reject\;', + 'v=DMARC1\\; p=reject\\;', ], }, ) @@ -119,7 +120,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'ttl': 86400, 'values': [ 'v=spf1 a ip4:1.2.3.4 ip6:2001:0db8:85a3:0000:0000:8a2e:0370:7334 -all', - 'v=DMARC1\; p=reject\;', + 'v=DMARC1\\; p=reject\\;', ], }, ) @@ -137,7 +138,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'ttl': 86400, 'values': [ 'v=spf1 a mx exists:example.com a a a a a a a a ~all', - 'v=DMARC1\; p=reject\;', + 'v=DMARC1\\; p=reject\\;', ], }, ) @@ -156,7 +157,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'ttl': 86400, 'values': [ 'v=spf1 include:example.com -all', - 'v=DMARC1\; p=reject\;', + 'v=DMARC1\\; p=reject\\;', ], }, ) @@ -183,7 +184,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'ttl': 86400, 'values': [ 'v=spf1 include:example.com -all', - 'v=DMARC1\; p=reject\;', + 'v=DMARC1\\; p=reject\\;', ], }, ) @@ -209,7 +210,7 @@ class TestSpfDnsLookupProcessor(TestCase): 'ttl': 86400, 'values': [ 'v=spf1 include:example.com -all', - 'v=DMARC1\; p=reject\;', + 'v=DMARC1\\; p=reject\\;', ], }, ) From 1bb672ed05b483748efbc00193034c3648143b4f Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Mon, 20 Feb 2023 18:24:47 +0000 Subject: [PATCH 12/13] Use `List[str]` to support older versions of Python --- octodns/processor/spf.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 35a395a..095e87c 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -3,7 +3,7 @@ # from logging import getLogger -from typing import Optional +from typing import List, Optional import dns.resolver from dns.resolver import Answer @@ -58,7 +58,7 @@ class SpfDnsLookupProcessor(BaseProcessor): super().__init__(name) def _get_spf_from_txt_values( - self, record: Record, values: list[str] + self, record: Record, values: List[str] ) -> Optional[str]: self.log.debug( f"_get_spf_from_txt_values: record={record.fqdn} values={values}" @@ -79,7 +79,7 @@ class SpfDnsLookupProcessor(BaseProcessor): return spf[0] - def _process_answer(self, answer: Answer) -> list[str]: + def _process_answer(self, answer: Answer) -> List[str]: values = [] for value in answer: @@ -90,7 +90,7 @@ class SpfDnsLookupProcessor(BaseProcessor): return values def _check_dns_lookups( - self, record: Record, values: list[str], lookups: int = 0 + self, record: Record, values: List[str], lookups: int = 0 ) -> int: self.log.debug( f"_check_dns_lookups: record={record.fqdn} values={values} lookups={lookups}" From ab64a6b0e45638e47dd86773162a8434bf78e7ba Mon Sep 17 00:00:00 2001 From: Samuel Parkinson Date: Mon, 20 Feb 2023 18:31:25 +0000 Subject: [PATCH 13/13] Replace use of `removeprefix` with slicing --- octodns/processor/spf.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/octodns/processor/spf.py b/octodns/processor/spf.py index 095e87c..6867a91 100644 --- a/octodns/processor/spf.py +++ b/octodns/processor/spf.py @@ -101,7 +101,7 @@ class SpfDnsLookupProcessor(BaseProcessor): if spf is None: return lookups - terms = spf.removeprefix('v=spf1 ').split(' ') + terms = spf[len('v=spf1 ') :].split(' ') for term in terms: if lookups > 10: @@ -120,7 +120,7 @@ class SpfDnsLookupProcessor(BaseProcessor): # The include mechanism can result in further lookups after resolving the DNS record if term.startswith('include:'): - domain = term.removeprefix('include:') + domain = term[len('include:') :] answer = dns.resolver.resolve(domain, 'TXT') answer_values = self._process_answer(answer) lookups = self._check_dns_lookups(