From c74911771505390bdd15c6cb42b14e31842e4417 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 8 Nov 2023 11:45:15 +0100 Subject: [PATCH 1/4] fix: return error when multiple valid DNSLink records --- namesys/dns_resolver.go | 30 ++++++++++++++++++++++++------ namesys/interface.go | 7 +++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/namesys/dns_resolver.go b/namesys/dns_resolver.go index ef46ce399..1d94c8e05 100644 --- a/namesys/dns_resolver.go +++ b/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" ) @@ -138,11 +139,12 @@ 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 @@ -150,16 +152,32 @@ func workDomain(ctx context.Context, r *DNSResolver, name string, res chan Async 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) { diff --git a/namesys/interface.go b/namesys/interface.go index 21c93126c..1befee855 100644 --- a/namesys/interface.go +++ b/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 ( From bfa6ff902138523ceb79e43b8789d00fb086055e Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 8 Nov 2023 11:50:20 +0100 Subject: [PATCH 2/4] feat!: remove support for legacy root dnslink, plumb errors --- namesys/dns_resolver.go | 64 ++++++++++-------------------------- namesys/dns_resolver_test.go | 52 +++++++++++++++-------------- 2 files changed, 45 insertions(+), 71 deletions(-) diff --git a/namesys/dns_resolver.go b/namesys/dns_resolver.go index 1d94c8e05..867b1b574 100644 --- a/namesys/dns_resolver.go +++ b/namesys/dns_resolver.go @@ -70,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 } }() diff --git a/namesys/dns_resolver_test.go b/namesys/dns_resolver_test.go index f45020a5c..e70ede174 100644 --- a/namesys/dns_resolver_test.go +++ b/namesys/dns_resolver_test.go @@ -2,7 +2,7 @@ package namesys import ( "context" - "fmt" + "net" "testing" "github.com/stretchr/testify/assert" @@ -52,7 +52,7 @@ 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 } @@ -60,33 +60,39 @@ func (m *mockDNS) lookupTXT(ctx context.Context, name string) (txt []string, err 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.multivalid.example.com.": { + "some stuff", + "dnslink=/ipns/dns1.example.com", + "dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD", + "masked 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 +101,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 +165,7 @@ 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/multivalid.example.com", 2, "", ErrMultipleDNSLinkRecords}, {"/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}, From 8a02441195b818ad560493f3b6eb67226559dec0 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 8 Nov 2023 11:52:08 +0100 Subject: [PATCH 3/4] changelog: add namesys entries --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30d50e9d6..db6859abf 100644 --- a/CHANGELOG.md +++ b/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 From d2776094b99c60790695f711b87617db5aa14784 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Fri, 10 Nov 2023 15:36:12 +0100 Subject: [PATCH 4/4] test: positive and negative test for multi dnslink --- namesys/dns_resolver_test.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/namesys/dns_resolver_test.go b/namesys/dns_resolver_test.go index e70ede174..c174a590a 100644 --- a/namesys/dns_resolver_test.go +++ b/namesys/dns_resolver_test.go @@ -80,11 +80,17 @@ func newMockDNS() *mockDNS { "dnslink=/ipns/dns1.example.com", "masked dnslink=/ipns/example.invalid", }, - "_dnslink.multivalid.example.com.": { + "_dnslink.multi-invalid.example.com.": { "some stuff", - "dnslink=/ipns/dns1.example.com", + "dnslink=/ipns/dns1.example.com", // we must error when >1 value with /ipns or /ipfs exists "dnslink=/ipfs/QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD", - "masked dnslink=/ipns/example.invalid", + "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", @@ -165,7 +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/multivalid.example.com", 2, "", ErrMultipleDNSLinkRecords}, + {"/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},