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.

Sign up for GitHub

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

Jump to bottom

feat: support DNSLink on non-ICANN DNS names #8071

Merged
merged 2 commits into from
May 13, 2021
Merged

feat: support DNSLink on non-ICANN DNS names #8071

merged 2 commits into from
May 13, 2021

Conversation

cong-or
Copy link
Contributor

@cong-or cong-or commented Apr 13, 2021 •
edited by lidel
Loading

Closes #8060

I switched IsDomain() from jbenet/go-is-domain to IsDomainName from github.com/miekg/dns as suggested.

I had to change one of the tests for everything to pass. This may be an error on my side. However, I could not understand why http://k51qzi5uqu5di608geewp3nqkg0bpujoasmka7ftkyxgcm3fh1aroup0gsdrna.ipns.localhost/ was expected?

lidel
lidel requested changes Apr 20, 2021
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @mycorrhizas!

I think we need to make this smarter and replace IsDomainName with a helper function that uses IsDomainName internally, but first correctly detects valid PeerID and returns "false" before calling IsDomainName.

Otherwise, we get false-positives when a valid PeerID is used in subdomain (thats why your test did not pass).

Samples you can use for testing:

  • QmY3hE8xgFCjGcz6PHgnvJz5HZi1BaKRfPkn1ghZUcYMjD (legacy peerid multihash in base58btc)
  • 12D3KooWFB51PRY9BxcXSH6khFXw1BZeszeLDy7C8GciskqCTZn5 (new peerid multihash in base58btc)
  • k2k4r8l9ja7hkzynavdqup76ou46tnvuaqegbd04a4o1mpbsey0meucb (new peerid as CIDv1 in base36)

lmk if anything is unclear

core/corehttp/hostname_test.go Outdated Show resolved Hide resolved
core/corehttp/hostname.go Outdated Show resolved Hide resolved
@ipfs ipfs deleted a comment Apr 20, 2021
@ipfs ipfs deleted a comment Apr 20, 2021
@lidel lidel changed the title feat support non-ICANN DNS feat: support non-ICANN DNS Apr 21, 2021
@lidel lidel mentioned this pull request Apr 21, 2021
Merged
2 tasks
@lidel
Copy link
Member

lidel commented Apr 21, 2021 •
edited
Loading

Ok, things got a bit more complicated because we have various PRs in flight, all related to DNS and DNSLink.

I think we need to wait for:

I'll refactor this PR to be ready for them.

@ipfs ipfs deleted a comment from welcome bot Apr 21, 2021
@lidel lidel changed the title feat: support non-ICANN DNS feat: support DNSLink on non-ICANN DNS names Apr 21, 2021
@lidel lidel self-assigned this Apr 21, 2021
@lidel lidel self-requested a review April 22, 2021 01:56
@cong-or
Copy link
Contributor Author

cong-or commented Apr 24, 2021 •
edited
Loading

@lidel apologies for the late reply

thanks for explaining and resolving the issue with isDomainNameAndNotPeerID

@lidel lidel requested a review from aschmahmann April 26, 2021 22:17
lidel
lidel approved these changes Apr 26, 2021
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but as I did some work here this needs more eyes.

@aschmahmann as noted during today's triage, this is ready for your final 👀

FYSA ipfs/go-namesys#13 already landed in namesys, so this is the final piece of the puzzle.
We may want to merge this AFTER #8068 as it should be easier to rebase this one if needed, than the other way around.

@lidel lidel mentioned this pull request Apr 26, 2021
Closed
aschmahmann
aschmahmann approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM, we'll merge after the DoH PR

@aschmahmann aschmahmann added status/accepted This issue has been accepted status/blocked Unable to be worked further until needs are met labels Apr 27, 2021
@lidel lidel added this to the go-ipfs 0.9 milestone Apr 29, 2021
@BigLep BigLep added this to In Review in Maintenance Priorities - Go via automation Apr 30, 2021
@aschmahmann aschmahmann removed the status/blocked Unable to be worked further until needs are met label May 13, 2021
cong-or and others added 2 commits May 13, 2021 10:54
@cong-or @aschmahmann
feat support non-ICANN DNS
24dd662
@lidel @aschmahmann
refactor: add isDomainNameAndNotPeerID
28d4d9b 
This ensures we exclude valid PeerIDs from code paths that require
DNSLink names.

Ref.
#8071 (review)
@aschmahmann aschmahmann merged commit afa9899 into ipfs:master May 13, 2021
Maintenance Priorities - Go automation moved this from In Review to Done May 13, 2021
@aschmahmann aschmahmann mentioned this pull request May 14, 2021
Closed
71 tasks
@lidel lidel mentioned this pull request Jun 30, 2021
Closed
@cong-or cong-or deleted the feat/non-icann-dns branch February 15, 2022 08:55
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
@lidel @aschmahmann
refactor: add isDomainNameAndNotPeerID
f1c5bb9 
This ensures we exclude valid PeerIDs from code paths that require
DNSLink names.

Ref.
ipfs/kubo#8071 (review)


This commit was moved from ipfs/kubo@28d4d9b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidel lidel lidel approved these changes

@aschmahmann aschmahmann aschmahmann approved these changes

Assignees

@lidel lidel

Labels
status/accepted This issue has been accepted topic/dns topic/gateway Topic gateway
Projects
No open projects
Maintenance Priorities - Go
Done
Milestone
go-ipfs 0.9
Development

Successfully merging this pull request may close these issues.

Support non-ICANN DNSLink names
3 participants
@cong-or @lidel @aschmahmann