Merge pull request #3 from octodns/source-validate-dns

Add verify_dns_lookups option to SpfSource
octodns · Sep 13, 2023 · 82d6902 · 82d6902
2 parents 178ff45 + c318532
commit 82d6902
Show file tree

Hide file tree

Showing 5 changed files with 122 additions and 53 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 * octodns.processor.spf.SpfDnsLookupProcessor ported in to this module as
   octodns_spf.SpfDnsLookupProcessor, deprecated the octoDNS core version.
+* Add verify_dns_lookups option to SpfSource
 
 ## v0.0.1 - 2023-08-21 - Initial (Alpha) Release
 

diff --git a/README.md b/README.md
@@ -32,12 +32,13 @@ octodns-spf==0.0.1
 
 ### Configuration
 
-#### Options &amp; Defaults
+#### SpfSource
 
 ```yaml
 providers:
   spf-google:
     class: octodns_spf.SpfSource
+
     # See https://datatracker.ietf.org/doc/html/rfc7208#section-5 for the
     # details of the various mechinisms below. Each is an array of zero or more
     # items to be added to the SPF record. Mechinisms are specified in the order
@@ -49,6 +50,7 @@ providers:
     ip6_addresses: []
     includes: []
     exists: []
+
     # The "all" value to be appended onto the SPF value, there's not a clear
     # consensus on best practice here, but there does seem to be a slight leaning
     # towards hard-failing, "-all". Soft-fail can be enabled by setting this
@@ -57,16 +59,56 @@ providers:
     # See https://news.ycombinator.com/item?id=34344590 for some discussion
     # (default: false, hard fail)
     soft_fail: false
+
     # Wether or not this provider will merge it's configuration with any
     # prexisting SPF value in an APEX TXT record. If `false` an error will be
     # thrown. If `true` the existing values, wether from a previous SpfSource or
     # any other provider, will be preserved and this provider's config will be
     # appended onto each mechinism.
     merging_enabled: false
+
+    # The TTL of the TXT record when created by SpfSource. If instead a value
+    # is added to an existing record the TTL will be left as-is.
+    # (default: 3600)
     ttl: 3600
+
+    # Enable verification of the SPF value, specifically evaluating the number
+    # of DNS lookups required to fully resolve the value.
+    # (default: false)
+    verify_dns_lookups: false
+```
+
+#### SpfDnsLookupProcessor
+
+Verifies that SPF values in TXT records are valid.
+
+```yaml
+
+    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
 ```
 
-#### Read World Example
+#### Real World Examples
 
 A base that disables all email applied to all Zones
 
@@ -76,7 +118,8 @@ providers:
     class: octodns_spf.SpfSource
 ```
 
-A follow on source that will add Google Workspace's recommended config
+A follow on source that will add the recommended values for Google Workspace
+and Salesforce.
 
 ```yaml
 providers:
@@ -87,12 +130,13 @@ providers:
       - _spf.salesforce.com
     soft_fail: true
     merging_enabled: true
+    verify_dns_lookups: true
 ```
 
 Per https://support.google.com/a/answer/10684623?hl=en and
 https://help.salesforce.com/s/articleView?id=000382664&type=1
 
-Zones would have one or more of these providers added to their sources list
+Zones would have one or more of these providers added to their sources list.
 
 ```yaml
 zones:
@@ -118,6 +162,38 @@ zones:
   ...
 ```
 
+If instead you prefer to just utilize the SpfDnsLookupProcessor stand alone on
+records configured in other ways you can do so by enabling the processor.
+Alternatively the processor could be configured in the manager's global
+processors list.
+
+```yaml
+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.
+
+```yaml
+_spf:
+  octodns:
+    lenient: true
+  ttl: 86400
+  type: TXT
+  value: v=spf1 ptr ~all
+```
+
 ### Support Information
 
 #### Records
@@ -126,4 +202,9 @@ TXT
 
 ### Development
 
-See the [/script/](/script/) directory for some tools to help with the development process. They generally follow the [Script to rule them all](https://github.com/github/scripts-to-rule-them-all) pattern. Most useful is `./script/bootstrap` which will create a venv and install both the runtime and development related requirements. It will also hook up a pre-commit hook that covers most of what's run by CI.
+See the [/script/](/script/) directory for some tools to help with the
+development process. They generally follow the [Script to rule them
+all](https://github.com/github/scripts-to-rule-them-all) pattern. Most useful
+is `./script/bootstrap` which will create a venv and install both the runtime
+and development related requirements. It will also hook up a pre-commit hook
+that covers most of what's run by CI.
diff --git a/octodns_spf/processor.py b/octodns_spf/processor.py
@@ -9,7 +9,6 @@
 from dns.resolver import Answer
 
 from octodns.processor.base import BaseProcessor, ProcessorException
-from octodns.record.base import Record
 
 
 class SpfValueException(ProcessorException):
@@ -21,50 +20,17 @@ class SpfDnsLookupException(ProcessorException):
 
 
 class SpfDnsLookupProcessor(BaseProcessor):
-    # TODO: deprecate and move into octodns_spf
-    '''
-    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):
         self.log.debug(f"SpfDnsLookupProcessor: {name}")
-        self.log.warning(
-            'SpfDnsLookupProcessor is DEPRECATED in favor of the version relocated into octodns-spf and will be removed in 2.0'
-        )
         super().__init__(name)
 
     def _get_spf_from_txt_values(
-        self, record: Record, values: List[str]
+        self, fqdn: str, values: List[str]
     ) -> Optional[str]:
         self.log.debug(
-            f"_get_spf_from_txt_values: record={record.fqdn} values={values}"
+            f"_get_spf_from_txt_values: record={fqdn} values={values}"
         )
 
         # SPF values to validate will begin with 'v=spf1 '
@@ -77,7 +43,7 @@ def _get_spf_from_txt_values(
         # 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"
+                f"{fqdn} has more than one SPF value in the TXT record"
             )
 
         return spf[0]
@@ -92,14 +58,14 @@ def _process_answer(self, answer: Answer) -> List[str]:
 
         return values
 
-    def _check_dns_lookups(
-        self, record: Record, values: List[str], lookups: int = 0
+    def check_dns_lookups(
+        self, fqdn: str, values: List[str], lookups: int = 0
     ) -> int:
         self.log.debug(
-            f"_check_dns_lookups: record={record.fqdn} values={values} lookups={lookups}"
+            f"check_dns_lookups: record={fqdn} values={values} lookups={lookups}"
         )
 
-        spf = self._get_spf_from_txt_values(record, values)
+        spf = self._get_spf_from_txt_values(fqdn, values)
 
         if spf is None:
             return lookups
@@ -109,12 +75,12 @@ def _check_dns_lookups(
         for term in terms:
             if lookups > 10:
                 raise SpfDnsLookupException(
-                    f"{record.fqdn} exceeds the 10 DNS lookup limit in the SPF record"
+                    f"{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"
+                    f"{fqdn} uses the deprecated ptr mechanism"
                 )
 
             # These mechanisms cost one DNS lookup each
@@ -126,9 +92,7 @@ def _check_dns_lookups(
                 domain = term[len('include:') :]
                 answer = dns.resolver.resolve(domain, 'TXT')
                 answer_values = self._process_answer(answer)
-                lookups = self._check_dns_lookups(
-                    record, answer_values, lookups
-                )
+                lookups = self.check_dns_lookups(fqdn, answer_values, lookups)
 
         return lookups
 
@@ -140,6 +104,6 @@ def process_source_zone(self, zone, *args, **kwargs):
             if record._octodns.get('lenient'):
                 continue
 
-            self._check_dns_lookups(record, record.values, 0)
+            self.check_dns_lookups(record.fqdn, record.values, 0)
 
         return zone
diff --git a/octodns_spf/source.py b/octodns_spf/source.py
@@ -8,6 +8,8 @@
 from octodns.record import Record, RecordException
 from octodns.source.base import BaseSource
 
+from .processor import SpfDnsLookupProcessor
+
 __VERSION__ = '0.0.1'
 
 
@@ -177,10 +179,11 @@ def __init__(
         soft_fail=False,
         merging_enabled=False,
         ttl=DEFAULT_TTL,
+        verify_dns_lookups=False,
     ):
         self.log = getLogger(f'SpfSource[{id}]')
         self.log.info(
-            '__init__: id=%s, a_records=%s, mx_records=%s, ip4_addresses=%s, ip6_addresses=%s, includes=%s, exists=%s, soft_fail=%s, merging_enabled=%s, ttl=%d',
+            '__init__: id=%s, a_records=%s, mx_records=%s, ip4_addresses=%s, ip6_addresses=%s, includes=%s, exists=%s, soft_fail=%s, merging_enabled=%s, ttl=%d, verify_dns_lookups=%s',
             id,
             a_records,
             mx_records,
@@ -191,6 +194,7 @@ def __init__(
             soft_fail,
             merging_enabled,
             ttl,
+            verify_dns_lookups,
         )
         super().__init__(id)
         self.a_records = a_records
@@ -216,6 +220,11 @@ def __init__(
         )
         self.log.debug('__init__:   spf=%s', self.spf_value)
 
+        if verify_dns_lookups:
+            SpfDnsLookupProcessor(self.id).check_dns_lookups(
+                f'<{self.id}>', [self.spf_value]
+            )
+
     def list_zones(self):
         # we're a specialized provider and never originate any zones ourselves.
         return []

diff --git a/tests/test_source_octodns_spf.py b/tests/test_source_octodns_spf.py
@@ -8,6 +8,7 @@
 from octodns.zone import Zone
 
 from octodns_spf import SpfSource
+from octodns_spf.processor import SpfDnsLookupException
 from octodns_spf.source import (
     SpfException,
     _build_spf,
@@ -563,3 +564,16 @@ def test_merging(self):
     def test_list_zones(self):
         # hard-coded [] so not much to do here
         self.assertEqual([], self.no_mail.list_zones())
+
+    def test_verify_dns_lookups(self):
+        a_records = [f'a_{i}.unit.tests.' for i in range(11)]
+
+        # too many lookups, but no verify so we're good
+        source = SpfSource('test', a_records=a_records)
+        self.assertTrue(source)
+
+        # too many lookups, verify is enabled so should blow up
+        with self.assertRaises(SpfDnsLookupException):
+            source = SpfSource(
+                'test', a_records=a_records, verify_dns_lookups=True
+            )