fix(namesys): handling of multiple DNSLink TXT records

CHANGELOG.md

@@ -18,9 +18,12 @@ The following emojis are used to highlight certain changes:
 
 ### Changed
 
+* 🛠 `boxo/namesys`: now fails when multiple valid DNSLink entries are found for the same domain. This used to cause undefined behavior before. Now, we return an error, according to the [specification](https://dnslink.dev/).
+
 ### Removed
 
 * 🛠 `boxo/gateway`: removed support for undocumented legacy `ipfs-404.html`. Use [`_redirects`](https://specs.ipfs.tech/http-gateways/web-redirects-file/) instead.
+* 🛠 `boxo/namesys`: removed support for legacy DNSLink entries at the root of the domain. Use [`_dnslink.` TXT record](https://docs.ipfs.tech/concepts/dnslink/) instead.
 
 ### Fixed
 

namesys/dns_resolver.go

@@ -12,6 +12,7 @@ import (
 	path "github.com/ipfs/boxo/path"
 	"github.com/ipfs/go-cid"
 	dns "github.com/miekg/dns"
+	"github.com/samber/lo"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 )
@@ -69,61 +70,31 @@ func (r *DNSResolver) resolveOnceAsync(ctx context.Context, p path.Path, options
 		fqdn += "."
 	}
 
-	rootChan := make(chan AsyncResult, 1)
-	go workDomain(ctx, r, fqdn, rootChan)
-
-	subChan := make(chan AsyncResult, 1)
-	go workDomain(ctx, r, "_dnslink."+fqdn, subChan)
+	resChan := make(chan AsyncResult, 1)
+	go workDomain(ctx, r, "_dnslink."+fqdn, resChan)
 
 	go func() {
 		defer close(out)
 		ctx, span := startSpan(ctx, "DNSResolver.ResolveOnceAsync.Worker")
 		defer span.End()
 
-		var rootResErr, subResErr error
-		for {
-			select {
-			case subRes, ok := <-subChan:
-				if !ok {
-					subChan = nil
-					break
-				}
-				if subRes.Err == nil {
-					p, err := joinPaths(subRes.Path, p)
-					emitOnceResult(ctx, out, AsyncResult{Path: p, LastMod: time.Now(), Err: err})
-					// Return without waiting for rootRes, since this result
-					// (for "_dnslink."+fqdn) takes precedence
-					return
-				}
-				subResErr = subRes.Err
-			case rootRes, ok := <-rootChan:
-				if !ok {
-					rootChan = nil
-					break
-				}
-				if rootRes.Err == nil {
-					p, err := joinPaths(rootRes.Path, p)
-					emitOnceResult(ctx, out, AsyncResult{Path: p, LastMod: time.Now(), Err: err})
-					// Do not return here.  Wait for subRes so that it is
-					// output last if good, thereby giving subRes precedence.
-				} else {
-					rootResErr = rootRes.Err
-				}
-			case <-ctx.Done():
-				return
+		select {
+		case subRes, ok := <-resChan:
+			if !ok {
+				break
 			}
-			if subChan == nil && rootChan == nil {
-				// If here, then both lookups are done
-				//
-				// If both lookups failed due to no TXT records with a
-				// dnslink, then output a more specific error message
-				if rootResErr == ErrResolveFailed && subResErr == ErrResolveFailed {
-					// Wrap error so that it can be tested if it is a ErrResolveFailed
-					err := fmt.Errorf("%w: _dnslink subdomain at %q is missing a TXT record (https://docs.ipfs.tech/concepts/dnslink/)", ErrResolveFailed, gopath.Base(fqdn))
-					emitOnceResult(ctx, out, AsyncResult{Err: err})
-				}
-				return
+			if subRes.Err == nil {
+				p, err := joinPaths(subRes.Path, p)
+				emitOnceResult(ctx, out, AsyncResult{Path: p, LastMod: time.Now(), Err: err})
+				// Return without waiting for rootRes, since this result
+				// (for "_dnslink."+fqdn) takes precedence
+			} else {
+				err := fmt.Errorf("DNSLink lookup for %q failed: %w", gopath.Base(fqdn), subRes.Err)
+				emitOnceResult(ctx, out, AsyncResult{Err: err})
 			}
+			return
+		case <-ctx.Done():
+			return
 		}
 	}()
 
@@ -138,28 +109,45 @@ func workDomain(ctx context.Context, r *DNSResolver, name string, res chan Async
 
 	txt, err := r.lookupTXT(ctx, name)
 	if err != nil {
-		if dnsErr, ok := err.(*net.DNSError); ok {
+		var dnsErr *net.DNSError
+		if errors.As(err, &dnsErr) {
 			// If no TXT records found, return same error as when no text
 			// records contain dnslink. Otherwise, return the actual error.
 			if dnsErr.IsNotFound {
-				err = ErrResolveFailed
+				err = ErrMissingDNSLinkRecord
 			}
 		}
 		// Could not look up any text records for name
 		res <- AsyncResult{Err: err}
 		return
 	}
 
+	// Convert all the found TXT records into paths. Ignore invalid ones.
+	var paths []path.Path
 	for _, t := range txt {
 		p, err := parseEntry(t)
 		if err == nil {
-			res <- AsyncResult{Path: p}
-			return
+			paths = append(paths, p)
 		}
 	}
 
-	// There were no TXT records with a dnslink
-	res <- AsyncResult{Err: ErrResolveFailed}
+	// Filter only the IPFS and IPNS paths.
+	paths = lo.Filter(paths, func(item path.Path, index int) bool {
+		return item.Namespace() == path.IPFSNamespace ||
+			item.Namespace() == path.IPNSNamespace
+	})
+
+	switch len(paths) {
+	case 0:
+		// There were no TXT records with a dnslink
+		res <- AsyncResult{Err: ErrMissingDNSLinkRecord}
+	case 1:
+		// Found 1 valid! Return it.
+		res <- AsyncResult{Path: paths[0]}
+	default:
+		// Found more than 1 IPFS/IPNS path.
+		res <- AsyncResult{Err: ErrMultipleDNSLinkRecords}
+	}
 }
 
 func parseEntry(txt string) (path.Path, error) {

namesys/dns_resolver_test.go

@@ -2,7 +2,7 @@ package namesys
 
 import (
 	"context"
-	"fmt"
+	"net"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -52,41 +52,53 @@ type mockDNS struct {
 func (m *mockDNS) lookupTXT(ctx context.Context, name string) (txt []string, err error) {
 	txt, ok := m.entries[name]
 	if !ok {
-		return nil, fmt.Errorf("no TXT entry for %s", name)
+		return nil, &net.DNSError{IsNotFound: true}
 	}
 	return txt, nil
 }
 
 func newMockDNS() *mockDNS {
 	return &mockDNS{
 		entries: map[string][]string{
-			"multihash.example.com.": {
+			"_dnslink.multihash.example.com.": {
 				"dnslink=QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
 			},
-			"ipfs.example.com.": {
+			"_dnslink.ipfs.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
 			},
 			"_dnslink.dipfs.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
 			},
-			"dns1.example.com.": {
+			"_dnslink.dns1.example.com.": {
 				"dnslink=/ipns/ipfs.example.com",
 			},
-			"dns2.example.com.": {
+			"_dnslink.dns2.example.com.": {
 				"dnslink=/ipns/dns1.example.com",
 			},
-			"multi.example.com.": {
+			"_dnslink.multi.example.com.": {
 				"some stuff",
 				"dnslink=/ipns/dns1.example.com",
 				"masked dnslink=/ipns/example.invalid",
 			},
-			"equals.example.com.": {
+			"_dnslink.multi-invalid.example.com.": {
+				"some stuff",
+				"dnslink=/ipns/dns1.example.com", // we must error when >1 value with /ipns or /ipfs exists
+				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
+				"broken dnslink=/ipns/example.invalid",
+			},
+			"_dnslink.multi-valid.example.com.": {
+				"some stuff",
+				"dnslink=/foo/bar", // duplicate dnslink= is fine as long it is not /ipfs or /ipns, which must be unique
+				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
+				"broken dnslink=/ipns/example.invalid",
+			},
+			"_dnslink.equals.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/=equals",
 			},
-			"loop1.example.com.": {
+			"_dnslink.loop1.example.com.": {
 				"dnslink=/ipns/loop2.example.com",
 			},
-			"loop2.example.com.": {
+			"_dnslink.loop2.example.com.": {
 				"dnslink=/ipns/loop1.example.com",
 			},
 			"_dnslink.dloop1.example.com.": {
@@ -95,46 +107,43 @@ func newMockDNS() *mockDNS {
 			"_dnslink.dloop2.example.com.": {
 				"dnslink=/ipns/loop1.example.com",
 			},
-			"bad.example.com.": {
+			"_dnslink.bad.example.com.": {
 				"dnslink=",
 			},
-			"withsegment.example.com.": {
+			"_dnslink.withsegment.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/sub/segment",
 			},
-			"withrecsegment.example.com.": {
+			"_dnslink.withrecsegment.example.com.": {
 				"dnslink=/ipns/withsegment.example.com/subsub",
 			},
-			"withtrailing.example.com.": {
+			"_dnslink.withtrailing.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/sub/",
 			},
-			"withtrailingrec.example.com.": {
+			"_dnslink.withtrailingrec.example.com.": {
 				"dnslink=/ipns/withtrailing.example.com/segment/",
 			},
-			"double.example.com.": {
-				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
-			},
 			"_dnslink.double.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
 			},
-			"double.conflict.com.": {
+			"_dnslink.double.conflict.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD",
 			},
 			"_dnslink.conflict.example.com.": {
 				"dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjE",
 			},
-			"fqdn.example.com.": {
+			"_dnslink.fqdn.example.com.": {
 				"dnslink=/ipfs/QmYvMB9yrsSf7RKBghkfwmHJkzJhW2ZgVwq3LxBXXPasFr",
 			},
-			"en.wikipedia-on-ipfs.org.": {
+			"_dnslink.en.wikipedia-on-ipfs.org.": {
 				"dnslink=/ipfs/bafybeiaysi4s6lnjev27ln5icwm6tueaw2vdykrtjkwiphwekaywqhcjze",
 			},
-			"custom.non-icann.tldextravaganza.": {
+			"_dnslink.custom.non-icann.tldextravaganza.": {
 				"dnslink=/ipfs/bafybeieto6mcuvqlechv4iadoqvnffondeiwxc2bcfcewhvpsd2odvbmvm",
 			},
-			"singlednslabelshouldbeok.": {
+			"_dnslink.singlednslabelshouldbeok.": {
 				"dnslink=/ipfs/bafybeih4a6ylafdki6ailjrdvmr7o4fbbeceeeuty4v3qyyouiz5koqlpi",
 			},
-			"www.wealdtech.eth.": {
+			"_dnslink.www.wealdtech.eth.": {
 				"dnslink=/ipns/ipfs.example.com",
 			},
 		},
@@ -162,6 +171,8 @@ func TestDNSResolution(t *testing.T) {
 		{"/ipns/multi.example.com", DefaultDepthLimit, "/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD", nil},
 		{"/ipns/multi.example.com", 1, "/ipns/dns1.example.com", ErrResolveRecursion},
 		{"/ipns/multi.example.com", 2, "/ipns/ipfs.example.com", ErrResolveRecursion},
+		{"/ipns/multi-invalid.example.com", 2, "", ErrMultipleDNSLinkRecords},
+		{"/ipns/multi-valid.example.com", DefaultDepthLimit, "/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD", nil},
 		{"/ipns/equals.example.com", DefaultDepthLimit, "/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD/=equals", nil},
 		{"/ipns/loop1.example.com", 1, "/ipns/loop2.example.com", ErrResolveRecursion},
 		{"/ipns/loop1.example.com", 2, "/ipns/loop1.example.com", ErrResolveRecursion},

namesys/interface.go

@@ -3,6 +3,7 @@ package namesys
 import (
 	"context"
 	"errors"
+	"fmt"
 	"time"
 
 	"github.com/ipfs/boxo/ipns"
@@ -22,6 +23,12 @@ var (
 
 	// ErrNoNamesys is an explicit error for when no [NameSystem] is provided.
 	ErrNoNamesys = errors.New("no namesys has been provided")
+
+	// ErrMultipleDNSLinkRecords signals that the domain had multiple valid DNSLink TXT entries.
+	ErrMultipleDNSLinkRecords = fmt.Errorf("%w: DNSLink lookup returned more than one IPFS content path; ask domain owner to remove duplicate TXT records", ErrResolveFailed)
+
+	// ErrMissingDNSLinkRecord signals that the domain has no DNSLink TXT entries.
+	ErrMissingDNSLinkRecord = fmt.Errorf("%w: DNSLink lookup could not find a TXT record (https://docs.ipfs.tech/concepts/dnslink/)", ErrResolveFailed)
 )
 
 const (