Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

feat: support non-ICANN DNSLink names #13

Merged
merged 3 commits into from
Apr 22, 2021
Merged

feat: support non-ICANN DNSLink names #13

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c0ad5e
feat: support non-ICANN DNSLink names
lidel Apr 21, 2021
3fcb2ca
test: non-ICANN DNS names
lidel Apr 21, 2021
29abfde
refactor: remove proquint.go
lidel Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (

path "github.com/ipfs/go-path"
opts "github.com/ipfs/interface-go-ipfs-core/options/namesys"
isd "github.com/jbenet/go-is-domain"
dns "github.com/miekg/dns"
)

// LookupTXTFunc is a generic type for a function that lookups TXT record values.
Expand Down Expand Up @@ -50,7 +50,7 @@ func (r *DNSResolver) resolveOnceAsync(ctx context.Context, name string, options
segments := strings.SplitN(name, "/", 2)
domain := segments[0]

if !isd.IsDomain(domain) {
if _, ok := dns.IsDomainName(domain); !ok {
out <- onceResult{err: fmt.Errorf("not a valid domain name: %s", domain)}
close(out)
return out
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ require (
github.com/ipfs/go-log v1.0.4
github.com/ipfs/go-path v0.0.9
github.com/ipfs/interface-go-ipfs-core v0.4.0
github.com/jbenet/go-is-domain v1.0.5
github.com/jbenet/goprocess v0.1.4
github.com/libp2p/go-libp2p v0.13.0
github.com/libp2p/go-libp2p-core v0.8.0
github.com/libp2p/go-libp2p-kad-dht v0.11.1
github.com/libp2p/go-libp2p-peerstore v0.2.6
github.com/libp2p/go-libp2p-record v0.1.3
github.com/libp2p/go-libp2p-testing v0.4.0
github.com/miekg/dns v1.1.41
github.com/multiformats/go-multiaddr v0.3.1
github.com/multiformats/go-multiaddr-dns v0.3.1
github.com/multiformats/go-multihash v0.0.14
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,6 @@ github.com/jackpal/go-nat-pmp v1.0.2/go.mod h1:QPH045xvCAeXUZOxsnwmrtiCoxIr9eob+
github.com/jbenet/go-cienv v0.0.0-20150120210510-1bb1476777ec/go.mod h1:rGaEvXB4uRSZMmzKNLoXvTu1sfx+1kv/DojUlPrSZGs=
github.com/jbenet/go-cienv v0.1.0 h1:Vc/s0QbQtoxX8MwwSLWWh+xNNZvM3Lw7NsTcHrvvhMc=
github.com/jbenet/go-cienv v0.1.0/go.mod h1:TqNnHUmJgXau0nCzC7kXWeotg3J9W34CUv5Djy1+FlA=
github.com/jbenet/go-is-domain v1.0.5 h1:r92uiHbMEJo9Fkey5pMBtZAzjPQWic0ieo7Jw1jEuQQ=
github.com/jbenet/go-is-domain v1.0.5/go.mod h1:xbRLRb0S7FgzDBTJlguhDVwLYM/5yNtvktxj2Ttfy7Q=
github.com/jbenet/go-temp-err-catcher v0.0.0-20150120210811-aac704a3f4f2/go.mod h1:8GXXJV31xl8whumTzdZsTt3RnUIiPqzkyf7mxToRCMs=
github.com/jbenet/go-temp-err-catcher v0.1.0 h1:zpb3ZH6wIE8Shj2sKS+khgRvf7T7RABoLk/+KKHggpk=
github.com/jbenet/go-temp-err-catcher v0.1.0/go.mod h1:0kJRvmDZXNMIiJirNPEYfhpPwbGVtZVWC34vc5WLsDk=
Expand Down
12 changes: 8 additions & 4 deletions namesys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
// DHT).
//
// Additionally, the /ipns/ namespace can also be used with domain names that
// use DNSLink (/ipns/my.domain.example, see https://dnslink.io) and proquint
// strings.
// use DNSLink (/ipns/<dnslink_name>, https://docs.ipfs.io/concepts/dnslink/)
// and proquint strings.
//
// The package provides implementations for all three resolvers.
package namesys
Expand All @@ -26,10 +26,10 @@ import (
dssync "github.com/ipfs/go-datastore/sync"
path "github.com/ipfs/go-path"
opts "github.com/ipfs/interface-go-ipfs-core/options/namesys"
isd "github.com/jbenet/go-is-domain"
ci "github.com/libp2p/go-libp2p-core/crypto"
peer "github.com/libp2p/go-libp2p-core/peer"
routing "github.com/libp2p/go-libp2p-core/routing"
dns "github.com/miekg/dns"
madns "github.com/multiformats/go-multiaddr-dns"
)

Expand Down Expand Up @@ -225,9 +225,13 @@ func (ns *mpns) resolveOnceAsync(ctx context.Context, name string, options opts.

if err == nil {
res = ns.ipnsResolver
} else if isd.IsDomain(key) {
} else if _, ok := dns.IsDomainName(key); ok {
res = ns.dnsResolver
} else {
// TODO: remove proquint?
// dns.IsDomainName(key) will return true for proquint strings,
// so this block is a dead code.
// (alternative is to move this before DNS check)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not particularly attached to the proquint resolver so if we needed to kill it then so be it.

Is the problem essentially that proquints are valid unqualified domain names since they can fit into a single label? IIUC proquints cannot contain ., so if we check for proquints first (and perhaps as an extra precaution prohibit resolving single label domain names) then we should probably be ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the future is for namesys and adding in support for other name resolution strategies, but I'm pretty sure that having them all prefixed as /ipns/<data> without any sort of prefix or identifier is just going to cause us trouble.

If we decide we're killing off proquints I'd like to know a bit of what the vision was for them first.

Copy link
Member Author

@lidel lidel Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

DNSLink on a single label domain name is a valid use case (private deployments with own DNS for petnames), so we can't block that.

It has been years, and I've never seen proquint being used in the real world. Not even once.
I suggest we remove proquint support and narrow down /ipns/ spec to be what it is today:

  1. PeerID
  2. DNS name

It is still manageable and easy to test (if not PeerID, then its DNS name), but we should not be adding anything more to /ipns/, because it is either increasing complexity on the browser vendor side or being a dead code (like proquints).

Copy link
Contributor

@aschmahmann aschmahmann Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

DNSLink on a single label domain name is valid use case (private deployments), so we can't block that.
...
(if not PeerID, then its DNS name)

If I have a DNS name that's a peer ID then it also conflicts, right (i.e. no error but we choose the IPNS name)?

but we should not be adding anything more to /ipns/

It depends on how you look at things. If you say /ipns/ is IPNS + DNSLink then perhaps that's fine. However, I suspect many people want improvements on IPNS which may mean some changes with the same (is it record type X or Y) kind of code. This would probably be hidden in go-ipns instead of here, but wanted to flag in case there's disagreement/confusion.

It has been years, and I've never seen proquint being used in the real world. Not even once.

As I said I've got no particular love for proquints, although the idea of encoding random data in a more visually/auditorily appealing way is nice (aside: it seems like it'd be more valuable as a multibase then a random thing here).

If @Stebalien has no issues with killing this off then 👍 from me.

Copy link
Member

Choose a reason for hiding this comment

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

I'd just remove them. If we want them back, we can pick a TLD and use that.

Copy link
Member

Choose a reason for hiding this comment

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

I.e., my-proquint.tld.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd just remove them. If we want them back, we can pick a TLD and use that.

Ack, I'll remove them shortly.

If I have a DNS name that's a peer ID then it also conflicts, right (i.e. no error but we choose the IPNS name)?

Correct. That is why PeerID lookup happens first (we prioritize cryptographic names over DNS ones).

If you say /ipns/ is IPNS + DNSLink then perhaps that's fine. However, I suspect many people want improvements on IPNS which may mean some changes with the same (is it record type X or Y) kind of code. This would probably be hidden in go-ipns instead of here, but wanted to flag in case there's disagreement/confusion.

Yes, if we ever want to change something in IPNS we would do that "within the CID boundary" (could be an improvement to the existing codec, or something new). We need to be mindful that we now have an ecosystem of things, and the way /ipns/ and ipns:// work is already in the process of slow ossification. We no longer can assume the value is passed as-is.

In real life people do some I/O validations, and that means ipns:// is limited to:

  • a CID of a libp2p-key (cryptographic), some people already normalize it to CIDv1Base36 with libp2p-key codec.
  • a DNS name with a valid DNSLink record (human-readable)

Last time I checked proquints won't even pass validation present in places like ENS.

aside: it seems like [proquints] be more valuable as a multibase than a random thing here

Yes, that was also my feeling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed: #13 (review)

Copy link
Member

Choose a reason for hiding this comment

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

multiformats/multibase#78

res = ns.proquintResolver
}

Expand Down