Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

namesys: error when more than one IPFS-related DNSLink record exist #507

Closed
lidel opened this issue Nov 5, 2023 · 4 comments
Closed

namesys: error when more than one IPFS-related DNSLink record exist #507

lidel opened this issue Nov 5, 2023 · 4 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up

Comments

@lidel
Copy link
Member

lidel commented Nov 5, 2023

Ref. ipfs/distributions#1053

In cases like the above, we should not silently grab the first record, but instead,

  • error when a domain name has more than one DNSLink TXT record starting with one of IPFS namespaces (dnslink=/ipfs/ or dnslink=/ipns/)
    • caveat: ignore records that use different namespaces (I've seen folks leveraging DNSLink for things other than IPFS).
Error: DNSLink lookup for "{domain}" returned more than one IPFS content path; ask domain owner to remove duplicate TXT record
@lidel lidel added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Nov 5, 2023
@lidel lidel moved this to 🥞 Todo in IPFS Shipyard Team Nov 5, 2023
@lidel
Copy link
Member Author

lidel commented Nov 5, 2023

@hacdias since you've recently been looking into boxo/namesys, would appreciate your insight: is this an easy fix or a more involved refactor? (I've put this as 0.25 item for now)

@Jorropo Jorropo added P1 High: Likely tackled by core team if no one steps up and removed need/triage Needs initial labeling and prioritization labels Nov 6, 2023
@Jorropo
Copy link
Contributor

Jorropo commented Nov 6, 2023

Triage notes: if this takes more than a few hours to do then it's P2.

@hacdias
Copy link
Member

hacdias commented Nov 7, 2023

I think this might be easier than expected, and I also want to raise another issue.

We check both _dnslink.{fqdn} and {fqdn}, giving priority to the former

See:

rootChan := make(chan AsyncResult, 1)
go workDomain(ctx, r, fqdn, rootChan)
subChan := make(chan AsyncResult, 1)
go workDomain(ctx, r, "_dnslink."+fqdn, subChan)

The specification does not define DNSLink entries in the root domain. Should we remove this? This was likely done for some backwards compatibility before the DNSLink specification was created: https://dnslink.dev/

Duplicated entries for same namespace

Seems like we can just check here:

for _, t := range txt {
p, err := parseEntry(t)
if err == nil {
res <- AsyncResult{Path: p}
return
}
}

It seems that, at the moment, we're just parsing the first TXT entry we find, no matter the namespace. It seems to me that if a domain had 3 DNSLink records for 3 different namespaces (valid), we'd only pick one and not even choose the one we support.

This brings another question: which namespace does Boxo prioritize? We support IPNS and IPFS. In case it has both, which do we choose?

@lidel
Copy link
Member Author

lidel commented Nov 7, 2023

The specification does not define DNSLink entries in the root domain. Should we remove this? This was likely done for some backwards compatibility before the DNSLink specification was created: https://dnslink.dev/

Yes, this was legacy behavior which we REMOVED from the spec to push people towards publishing on _dnslink subdomain. We kept it because removal would break ENS, but the resolver at https://resolver.cloudflare-eth.com/dns-query now returns TXT from _dnslink as well, so no longer a blocker.

We could remove support for legacy behavior, it is very old and may break someone's website, but should be an easy fix for them because we provide meaningful error now:

could not resolve name: _dnslink subdomain at "example.net." is missing a TXT record (https://docs.ipfs.tech/concepts/dnslink/)

That being said, I would not change do this unless it makes fix for this issue easier.

It seems that, at the moment, we're just parsing the first TXT entry we find, no matter the namespace.[..]
This brings another question: which namespace does Boxo prioritize? We support IPNS and IPFS. In case it has both, which do we choose?

In my mind, IPNS and IPFS values are equivalent, and having two should produce error.

The desired behavior would be:

  1. Make DNS lookup and read TXT records.
  2. Keep TXT records with values that start with dnslink=/ipfs/ OR dnslink=/ipns/ (ignore everything else)
  3. Error if there is no record left, or if there is more than one.

This way we allow non-ipfs records to coexist, but ensure there is only one for boxo/namesys user to act on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up
Projects
No open projects
Archived in project
Development

No branches or pull requests

3 participants