ipfs / go-dnslink

dnslink resolution in go-ipfs
MIT License
55 stars 17 forks source link

enable _dnslink resolution #13

Closed bonedaddy closed 3 years ago

bonedaddy commented 5 years ago

Changes

Notes

Right now this is using a fork of jbenet/go-is-domain since that repo is pretty stale, and I wasn't sure if/when this PR would be merged. The fork also contains updated TLDs from IANA using version 2019031400.

Should the PR in jbenet/go-is-domain be merged, the dependencies for this PR will need to be updated.

bonedaddy commented 5 years ago

If desired I can include a fix for issue#3 in this PR as well. For my own purposes I've written that functionality, see here if this would be something I should include here

bonedaddy commented 5 years ago

@Stebalien Ah okay thanks for the clarification, I was a little unsure as to the difference. For some additional clarification:

/dns (or more appropriately) /dnslink is used when resolving DNSLink records, whereas /dnsaddr, /dns4 and /dns6 are used within libp2p to indicate that to get the IP of the peer to connect to, you must first resolve the DNS record to get the IP address?

Also the part of your comment If you want to use this library, you'll need to fix this. I presume this means the library needs to be adjusted to handle /dnslink as well as /dns or rather, change to only support /dnslink

bonedaddy commented 5 years ago

As per your comment on the go-is-domain PR, the only place I can think of placing this would be here.

The main issue I encountered when attempting to resolve _dnslink records was that isDomain() would trip on it, presumably because the regex doesn't allow use of _.

Stebalien commented 5 years ago

Ah, so, actually, that issue was a bit underspecified. It isn't about resolving /dnslink/_dnslink.foo.com. It's about resolving /dnslink/foo.com when the dnslink TXT record is attached to _dnslink.foo.com. That is, the resolution logic should be:

Given /dnslink/foo.com:

  1. Lookup _dnslink.foo.com. If it has a TXT record with a value dnslink=$PATH, resolve to $PATH.
  2. Lookup foo.com and follow the same procedure as above.
bonedaddy commented 5 years ago

Ah okay thanks for the clarification, I'll go ahead and make those changes :ok_hand:

Stebalien commented 5 years ago

Thanks! And thanks for picking this up!

bonedaddy commented 5 years ago

No problem happy to help! I'm about 99.98% sure that the last commits of mine added the correct functionality, let me know if there's anything i need to change.

I also did a general find+replace to change any mentions of /dns/ -> /dnslink/

Stebalien commented 5 years ago

I'm not seeing the _dnslink resolution part.

bonedaddy commented 5 years ago

Ah yes apologies. I believe the following resolveOnce change will satisfy the _dnslink resolution (unless im misunderstanding your comment), let me know if this works and i'll commit the change:

// resolveOnce implements resolver.
func (r *Resolver) resolveOnce(domain string) (p string, err error) {
    r.setDefaults()
    // if given _dnslink.foo.com, trim _dnslink. before validating domain
    if !isd.IsDomain(strings.TrimPrefix(domain, "_dnslink.")) {
        return "", ErrInvalidDomain
    }
    txt, err := r.lookupTXT(domain)
    if err != nil {
        return "", err
    }
    err = ErrResolveFailed
    for _, t := range txt {
        p, err = ParseTXT(t)
        if err == nil {
            return p, nil
        }
    }

    return "", err
}

which results in the following output:

$ dnslink temporal.cloud
/ipfs/QmVqeSeC9DGVY2ui8VYE1dZ6EtyT63GqCDbkaniaiXgjGG
$ dnslink _dnslink.temporal.cloud
/ipfs/QmVqeSeC9DGVY2ui8VYE1dZ6EtyT63GqCDbkaniaiXgjGG
Stebalien commented 5 years ago

Sorry, that's not what I meant. The user should never try to run dnslink _dnslink.temporal.cloud. The user will run dnslink temporal.cloud and the dnslink command should look for a dnslink TXT record on _dnslink.temporal.cloud and, if that doesn't work, on temporal.cloud.

That way, we can put the dnslink on the _dnslink subdomain and avoid polluting the root domain. The issue here is that the most specific DNS query we can make is "TXT". If everyone attaches their special TXT records to the root domain, the responses will get massive. That's why we support (and now encourage) putting the TXT record on the _dnslink subdomain (which isn't a valid hostname so it'll never conflict with any actual website subdomain).

bonedaddy commented 5 years ago

Ah I see thanks for the clarification. In that case, this should be the suitable solution

// resolveOnce implements resolver.
func (r *Resolver) resolveOnce(domain string) (p string, err error) {
    r.setDefaults()
    if !isd.IsDomain(domain) {
        return "", ErrInvalidDomain
    }
    var txt []string
    // first attempt lookup on _dnslink.domain
    // if this fails, we'll then do a generalized
    // search against all TXT records for given domain
    // and if that fails, return an error
    txt, err = r.lookupTXT(fmt.Sprintf("_dnslink.%s", domain))
    if err != nil {
        txt, err = r.lookupTXT(domain)
        if err != nil {
            return "", err
        }
    }
    err = ErrResolveFailed
    for _, t := range txt {
        p, err = ParseTXT(t)
        if err == nil {
            return p, nil
        }
    }

    return "", err
}
Stebalien commented 5 years ago

That looks correct. Ideally, it would query both in parallel, preferring the result from _dnslink.domain, but we can handle that later.

bonedaddy commented 5 years ago

Sounds good! I've pushed the change

bonedaddy commented 5 years ago

Not quite sure why travis is failing to build after i switched the dependency to jbenet/go-is-domain :thinking:

autonome commented 4 years ago

@bonedaddy @Stebalien is this work still relevant? Any idea what the test failure is?

lidel commented 3 years ago

/dns/dnslink and _dnslink subdomain support changes are still relevant to work @martinheidegger is doing, but it's been a while, and we are moving dnslink under https://github.com/dnslink-std, so we need to tackle those things in fresh PR.

(lmk if you want to pick this up, or should we close it as stale and fill a new one)

martinheidegger commented 3 years ago

:wave: Yes, and hi! This is still relevant. I have been working on a test suite for dnslink implementations and it currently uses the /dns/ prefix - the reason for this is because while most implementations simply ignore the prefix 😓, go-dnslink seems to be the only implementation (?0 that uses /dns/ explicitly: https://github.com/ipfs/go-dnslink/blob/0626e50cfeaf0cd304eb0017de996e8cacb08540/dnslink.go#L220 - however it may be a good idea to support both as the documentation mentions /dnslink/ everywhere. I opened an issue regarding this: https://github.com/dnslink-std/test/issues/9