git » spf » commit 0017db6

Replace workaround to detect NXDOMAIN with actual checks

author Alberto Bertogli
2022-08-17 22:37:57 UTC
committer Alberto Bertogli
2022-08-27 16:49:25 UTC
parent 2ac5fa58d18d80465dcd039aba3df3d82c54ead4
Replace workaround to detect NXDOMAIN with actual checks

The RFC is clear that DNS errors other than RCODE 3 (NXDOMAIN) should
result in a "temperror":

> Several mechanisms rely on information fetched from the DNS.  For
> these DNS queries, except where noted, if the DNS server returns an
> error (RCODE other than 0 or 3) or the query times out, the mechanism
> stops and the topmost check_host() returns "temperror".  If the server
> returns "Name Error" (RCODE 3), then evaluation of the mechanism
> continues as if the server returned no error (RCODE 0) and zero answer
> records.

https://www.rfc-editor.org/rfc/rfc7208#section-5

Originally, Go lacked the ability to distinguish RCODE 3 (NXDOMAIN) from
other causes, so the code had to assume all DNS permanent errors were
NXDOMAIN as a (very imperfect) workaround.

That causes it to treat legitimate errors in the same way as NXDOMAIN,
which is incorrect.

Now that Go has implemented an API to detect NXDOMAIN (and we already
use it), we can replace those workarounds with proper checks, and
properly respond temperror when we should.

Thanks to Matthias Schneider <ms@wck.biz> for reporting this issue and
providing an alternative patch in
https://github.com/albertito/spf/pull/8 !
spf.go +13 -16
spf_test.go +13 -8
testdata/blitirispf-tests.yml +2 -2 
diff --git a/spf.go b/spf.go
index 431b931..d36826a 100644
--- a/spf.go
+++ b/spf.go
@@ -565,10 +565,10 @@ func (r *resolution) ptrField(res Result, field, domain string) (bool, Result, e
 		r.checkVoidLookup(len(ns), err)
 		if err != nil {
 			// https://tools.ietf.org/html/rfc7208#section-5
-			if isTemporary(err) {
-				return true, TempError, err
+			if isNotFound(err) {
+				return false, "", err
 			}
-			return false, "", err
+			return true, TempError, err
 		}
 
 		// Only take the first 10 names, ignore the rest.
@@ -628,10 +628,10 @@ func (r *resolution) existsField(res Result, field, domain string) (bool, Result
 	r.checkVoidLookup(len(ips), err)
 	if err != nil {
 		// https://tools.ietf.org/html/rfc7208#section-5
-		if isTemporary(err) {
-			return true, TempError, err
+		if isNotFound(err) {
+			return false, "", err
 		}
-		return false, "", err
+		return true, TempError, err
 	}
 
 	// Exists only counts if there are IPv4 matches.
@@ -758,10 +758,10 @@ func (r *resolution) aField(res Result, field, domain string) (bool, Result, err
 	r.checkVoidLookup(len(ips), err)
 	if err != nil {
 		// https://tools.ietf.org/html/rfc7208#section-5
-		if isTemporary(err) {
-			return true, TempError, err
+		if isNotFound(err) {
+			return false, "", err
 		}
-		return false, "", err
+		return true, TempError, err
 	}
 	for _, ip := range ips {
 		if ipMatch(r.ip, ip.IP, masks) {
@@ -791,10 +791,10 @@ func (r *resolution) mxField(res Result, field, domain string) (bool, Result, er
 	r.checkVoidLookup(len(mxs), err)
 	if err != nil {
 		// https://tools.ietf.org/html/rfc7208#section-5
-		if isTemporary(err) {
-			return true, TempError, err
+		if isNotFound(err) {
+			return false, "", err
 		}
-		return false, "", err
+		return true, TempError, err
 	}
 
 	// There's an explicit maximum of 10 MX records per match.
@@ -812,10 +812,7 @@ func (r *resolution) mxField(res Result, field, domain string) (bool, Result, er
 			if isNotFound(err) {
 				continue
 			}
-			if isTemporary(err) {
-				return true, TempError, err
-			}
-			return false, "", err
+			return true, TempError, err
 		}
 		for _, ipaddr := range ips {
 			mxips = append(mxips, ipaddr.IP)
diff --git a/spf_test.go b/spf_test.go
index c2391c8..65d1a86 100644
--- a/spf_test.go
+++ b/spf_test.go
@@ -303,21 +303,26 @@ func TestDNSPermanentErrors(t *testing.T) {
 		IsTemporary: false,
 	}
 
-	// Domain "tmperr" will fail resolution with a temporary error.
-	dns.Errors["tmperr"] = dnsError
+	// Domain "permerr" will fail resolution with a permanent error.
+	dns.Errors["permerr"] = dnsError
 	dns.Errors["1.1.1.1"] = dnsError
-	dns.Mx["tmpmx"] = []*net.MX{mx("tmperr", 10)}
+	dns.Mx["permmx"] = []*net.MX{mx("permerr", 10)}
 	defaultTrace = t.Logf
 
 	cases := []struct {
 		txt string
 		res Result
 	}{
-		{"v=spf1 include:tmperr", PermError},
-		{"v=spf1 a:tmperr", Neutral},
-		{"v=spf1 mx:tmperr", Neutral},
-		{"v=spf1 ptr:tmperr", Neutral},
-		{"v=spf1 mx:tmpmx", Neutral},
+		// Top-level checks will return a permanent error.
+		{"v=spf1 include:permerr", PermError},
+
+		// RFC specifies that on any DNS error (other than NXDOMAIN),
+		// we must return TempError.
+		// https://www.rfc-editor.org/rfc/rfc7208#section-5
+		{"v=spf1 a:permerr", TempError},
+		{"v=spf1 mx:permerr", TempError},
+		{"v=spf1 ptr:permerr", TempError},
+		{"v=spf1 mx:permmx", TempError},
 	}
 
 	for _, c := range cases {
diff --git a/testdata/blitirispf-tests.yml b/testdata/blitirispf-tests.yml
index d0cf364..1f21a3e 100644
--- a/testdata/blitirispf-tests.yml
+++ b/testdata/blitirispf-tests.yml
@@ -106,10 +106,10 @@ tests:
   exists-perm-error:
     description: |
       Check that if, during an 'exists' forward resolution we get an error, we
-      fail the check.
+      return temperror.
     mailfrom: "foo@domain.net"
     host: 1.2.3.4
-    result: softfail
+    result: temperror
 zonedata:
   domain.net:
     - SPF: v=spf1 exists:lalala.com ~all