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

Conversation

lidel
Copy link
Member

@lidel lidel commented Apr 21, 2021

Part of ipfs/kubo#8060 (needed for unblocking ipfs/kubo#8071)

TODO:

  • remove ICANN safelist (github.com/jbenet/go-is-domain)
  • decide what to do with proquints (we 🪓 them)

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

LGTM.

It seems like it would be easy enough to add a unit test for this so the coverage check does not complain, oh, and so this is tested if it is not already.

namesys.go Outdated
Comment on lines 231 to 234
// 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

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

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.

Copy link
Member Author

@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.

As agreed in this thread, I removed proquint.go and related code (29abfde).

// so this block is a dead code.
// (alternative is to move this before DNS check)
res = ns.proquintResolver
out <- onceResult{err: fmt.Errorf("invalid IPNS root: %q", key)}
Copy link
Member Author

@lidel lidel Apr 21, 2021

Choose a reason for hiding this comment

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

I felt it is better to not default to "DNSLink not found" when input is not even a domain (eg. something like ..), so added a fallback error here.

@lidel lidel marked this pull request as ready for review April 22, 2021 12:45
@lidel
Copy link
Member Author

lidel commented Apr 22, 2021

This should be ready for final review.

Note: we don't have real end-to-end tests here, only small units, but we do have e2e in go-ipfs repo.

@Stebalien Stebalien merged commit 35a0a7b into feat/custom-resolver Apr 22, 2021
@Stebalien Stebalien deleted the feat/allow-non-icann-tlds branch April 22, 2021 22:25
vyzo pushed a commit that referenced this pull request Apr 23, 2021
guseggert pushed a commit to ipfs/boxo that referenced this pull request Dec 6, 2022
Jorropo pushed a commit to ipfs/go-libipfs-rapide that referenced this pull request Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants