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`