spf: fixup handling of NoRecordsFound during exists: check

Similar to the general purpose resolve function, we should map
NoRecordsFound to empty list(s) of results in the specialized
ip, mx and ptr lookup functions, otherwise we can cause rule
evaluation to terminate too soon.

crates/dns-resolver/src/resolver.rs

@@ -171,8 +171,8 @@ impl TestResolver {
         self
     }
 
-    pub fn with_txt(self, domain: &str, value: String) -> Self {
-        self.with_txt_multiple(domain, vec![value])
+    pub fn with_txt(self, domain: &str, value: impl Into<String>) -> Self {
+        self.with_txt_multiple(domain, vec![value.into()])
     }
 
     /// Add multiple separate TXT records for the specified domain
@@ -479,36 +479,36 @@ impl Resolver for HickoryResolver {
         let name = Name::from_utf8(host)
             .map_err(|err| DnsError::InvalidName(format!("invalid name {host}: {err}")))?;
 
-        self.inner
-            .lookup_ip(name)
-            .await
-            .map_err(|err| DnsError::from_resolve(&host, err))?
-            .into_iter()
-            .map(Ok)
-            .collect()
+        match self.inner.lookup_ip(name.clone()).await {
+            Ok(result) => Ok(result.into_iter().collect()),
+            Err(err) => match err.proto().map(|err| err.kind()) {
+                Some(ProtoErrorKind::NoRecordsFound { .. }) => Ok(vec![]),
+                _ => Err(DnsError::from_resolve(&name, err)),
+            },
+        }
     }
 
     async fn resolve_mx(&self, host: &str) -> Result<Vec<Name>, DnsError> {
         let name = Name::from_utf8(host)
             .map_err(|err| DnsError::InvalidName(format!("invalid name {host}: {err}")))?;
 
-        self.inner
-            .mx_lookup(name)
-            .await
-            .map_err(|err| DnsError::from_resolve(&host, err))?
-            .into_iter()
-            .map(|mx| Ok(mx.exchange().clone()))
-            .collect()
+        match self.inner.mx_lookup(name.clone()).await {
+            Ok(result) => Ok(result.into_iter().map(|mx| mx.exchange().clone()).collect()),
+            Err(err) => match err.proto().map(|err| err.kind()) {
+                Some(ProtoErrorKind::NoRecordsFound { .. }) => Ok(vec![]),
+                _ => Err(DnsError::from_resolve(&name, err)),
+            },
+        }
     }
 
     async fn resolve_ptr(&self, ip: IpAddr) -> Result<Vec<Name>, DnsError> {
-        self.inner
-            .reverse_lookup(ip)
-            .await
-            .map_err(|err| DnsError::from_resolve(&ip, err))?
-            .into_iter()
-            .map(|ptr| Ok(ptr.0))
-            .collect()
+        match self.inner.reverse_lookup(ip).await {
+            Ok(result) => Ok(result.into_iter().map(|ptr| ptr.0).collect()),
+            Err(err) => match err.proto().map(|err| err.kind()) {
+                Some(ProtoErrorKind::NoRecordsFound { .. }) => Ok(vec![]),
+                _ => Err(DnsError::from_resolve(&ip, err)),
+            },
+        }
     }
 
     async fn resolve(&self, name: Name, rrtype: RecordType) -> Result<Answer, DnsError> {

crates/kumo-spf/Cargo.toml

@@ -3,6 +3,12 @@ name = "kumo-spf"
 version = "0.1.0"
 edition = "2021"
 
+[features]
+# Enable this feature to run real DNS lookups.
+# Resulting tests may be flaky and, if the upstream
+# has changed their records, may be wrong.
+live-dns-tests = []
+
 [dependencies]
 dns-resolver = {path="../dns-resolver"}
 hickory-resolver = {workspace=true}

crates/kumo-spf/src/tests.rs

@@ -497,3 +497,70 @@ async fn initial_processing() {
     assert_eq!(result.disposition, SpfDisposition::None);
     assert_eq!(result.context, "no SPF records found for example.com");
 }
+
+/// This test is a little bit disingenuous, because the issue it is testing
+/// is impossible to reproduce with the TestResolver.  The issue was that
+/// the underlying resolver would propagate a NoRecordsFound hickory error
+/// as a DnsError::ResolveFailed instead of returning an empty list of
+/// ip addresses.  The error would essentially blow up the exists: rule which
+/// is defined as being OK in the face of having no matching records.
+#[tokio::test]
+async fn no_records_for_exists_should_not_block_otherwise_satisfied_eval() {
+    let resolver = TestResolver::default()
+        .with_txt(
+            "greenhouse.io",
+            "v=spf1 include:_spf.salesforce.com include:mg-spf.greenhouse.io ~all",
+        )
+        .with_txt(
+            "_spf.salesforce.com",
+            // Note that we don't provide any of these IP._spf.mta.salesforce.com
+            // A or AAAA entries, so the exists checks will all fail
+            "v=spf1 exists:%{i}._spf.mta.salesforce.com -all",
+        )
+        .with_txt(
+            "mg-spf.greenhouse.io",
+            "v=spf1 ip4:185.250.239.148 ip4:185.250.239.168 ip4:185.250.239.190 \
+                ip4:198.244.59.30 ip4:198.244.59.33 ip4:198.244.59.35 \
+                ip4:198.61.254.21 ip4:209.61.151.236 ip4:209.61.151.249 \
+                ip4:209.61.151.251 ip4:69.72.40.93 ip4:69.72.40.94/31 \
+                ip4:69.72.40.96/30 ip4:69.72.47.205 ~all",
+        );
+
+    let cx = SpfContext::new(
+        "sender@greenhouse.io",
+        "greenhouse.io",
+        "69.72.47.205".parse().unwrap(),
+    )
+    .unwrap();
+    let result = cx.check(&resolver, true).await;
+    eprintln!("{result:#?}");
+    assert_eq!(result.disposition, SpfDisposition::Pass);
+    assert_eq!(
+        result.context,
+        "matched 'include:mg-spf.greenhouse.io' directive"
+    );
+}
+
+/// This is the live-dns version of the above
+/// no_records_for_exists_should_not_block_otherwise_satisfied_eval test case
+/// that queries real DNS with a real resolver. Prior to the fix for this issue,
+/// this test would fail.
+#[cfg(feature = "live-dns-tests")]
+#[tokio::test]
+async fn live_no_records_for_exists_should_not_block_otherwise_satisfied_eval() {
+    use dns_resolver::HickoryResolver;
+    let resolver = HickoryResolver::new().unwrap();
+    let cx = SpfContext::new(
+        "sender@greenhouse.io",
+        "greenhouse.io",
+        "69.72.47.205".parse().unwrap(),
+    )
+    .unwrap();
+    let result = cx.check(&resolver, true).await;
+    eprintln!("{result:#?}");
+    assert_eq!(result.disposition, SpfDisposition::Pass);
+    assert_eq!(
+        result.context,
+        "matched 'include:mg-spf.greenhouse.io' directive"
+    );
+}

docs/changelog/main.md

@@ -55,3 +55,5 @@
  * smtp server would incorrectly return a 451 instead of a 452 status when
    `max_recipients_per_message` or `max_messages_per_connection` limits
    were exceeded.
+ * spf: a `NoRecordsFound` response from DNS during an `exists:` rule check
+   could cause the result to incorrectly be reported a `temperror`