From 67061e3a24dcce7fa83cf0a07ae37d76db8993bd Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Thu, 9 Oct 2025 08:47:09 +0100 Subject: [PATCH] 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 | 46 ++++++++++---------- crates/kumo-spf/Cargo.toml | 6 +++ crates/kumo-spf/src/tests.rs | 67 +++++++++++++++++++++++++++++ docs/changelog/main.md | 2 + 4 files changed, 98 insertions(+), 23 deletions(-) diff --git a/crates/dns-resolver/src/resolver.rs b/crates/dns-resolver/src/resolver.rs index c4571214..e6688842 100644 --- a/crates/dns-resolver/src/resolver.rs +++ b/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) -> 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, 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, 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 { diff --git a/crates/kumo-spf/Cargo.toml b/crates/kumo-spf/Cargo.toml index 905d90a0..a355eff5 100644 --- a/crates/kumo-spf/Cargo.toml +++ b/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} diff --git a/crates/kumo-spf/src/tests.rs b/crates/kumo-spf/src/tests.rs index a73cbb41..d5030634 100644 --- a/crates/kumo-spf/src/tests.rs +++ b/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" + ); +} diff --git a/docs/changelog/main.md b/docs/changelog/main.md index 7e650e16..1bbc3f15 100644 --- a/docs/changelog/main.md +++ b/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`