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

fix(namesys): handling of multiple DNSLink TXT records #510

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 8, 2023

This addresses #507. I made individual commits to be easy to follow.

I removed the legacy support for DNSLink entries at the root of the domain. This made it easier to plug in the errors from the DNS. My issue was that if both rootCh and subCh returned an error, and the error was different, which one to choose? Probably subCh, but that still felt fragile. I can maintain legacy support if we want it, but that will result in a bit more work.

I also added two new errors to the package, to make it easier to test against specific types of errors.

@hacdias hacdias requested a review from lidel November 8, 2023 10:54
@hacdias hacdias self-assigned this Nov 8, 2023
@hacdias hacdias requested a review from a team as a code owner November 8, 2023 10:54
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #510 (d277609) into main (49e0f39) will decrease coverage by 0.04%.
The diff coverage is 80.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
- Coverage   65.52%   65.49%   -0.04%     
==========================================
  Files         207      204       -3     
  Lines       25553    25469      -84     
==========================================
- Hits        16743    16680      -63     
+ Misses       7336     7319      -17     
+ Partials     1474     1470       -4     
Files Coverage Δ
namesys/interface.go 74.46% <ø> (ø)
namesys/dns_resolver.go 72.17% <80.55%> (-5.17%) ⬇️

... and 13 files with indirect coverage changes

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Removing undocumented legacy lookup is ok. People had multiple years to migrate to _dnslink, and the error message will be clear:

Error: DNSLink lookup for "google.com." failed: could not resolve name: DNSLink lookup could not find a TXT record (https://docs.ipfs.tech/concepts/dnslink/)

Added positive test for valid duplicates that we don't care about (making sure that is ignored correctly – we don't want to gatekeep DNSLink, people should be able to use it for non-IPFS namespaces if they want)

Tested it end-to-end against latest Kubo and works as expected:

$ dig +short TXT _dnslink.invalid-duplicate-dnslink-test.example.com.
"dnslink=/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR"
"dnslink=/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"

$ ipfs resolve /ipns/invalid-duplicate-dnslink-test.example.com
Error: DNSLink lookup for "invalid-duplicate-dnslink-test.example.com." failed: could not resolve name: DNSLink lookup returned more than one IPFS content path; ask domain owner to remove duplicate TXT records

$ dig +short TXT _dnslink.valid-duplicate-dnslink-test.example.com.
"dnslink=/foo/bar"
"dnslink=/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"

$ ipfs resolve /ipns/valid-duplicate-dnslink-test.example.com
/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi

@lidel lidel changed the title namesys: error with multiple DNSLink records fix(namesys): handling of multiple DNSLink TXT records Nov 10, 2023
@hacdias hacdias enabled auto-merge (rebase) November 10, 2023 15:12
@hacdias hacdias merged commit 50f8a08 into main Nov 10, 2023
13 checks passed
@hacdias hacdias deleted the fix-507 branch November 10, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants