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

Added custom DNS resolver with support for DNS-over-TLS and DNS-over-HTTPS #6560

Closed
wants to merge 7 commits into from

Conversation

ItalyPaleAle
Copy link

@ItalyPaleAle ItalyPaleAle commented Aug 7, 2019

This PR should fix #6532

In the current implementation, go-ipfs uses the machine's DNS resolver for querying TXT records used by DNSLink. As highlighted in #6532, this can come with some issues related to censorship, privacy, and support for TOR.

I've implemented a custom DNS resolver that supports:

  1. Traditional DNS queries (over plain-text UDP), but using a customizable DNS server
  2. DNS-over-TLS, with a custom DNS server
  3. DNS-over-HTTPS, with a custom DNS server

Note that the PR is still not complete. At the moment, I am missing:

  • Being able to pass the configuration for the DNS server from the config file
  • Adding tests

@ItalyPaleAle ItalyPaleAle marked this pull request as ready for review August 7, 2019 04:07
@ItalyPaleAle
Copy link
Author

Note: tests will fail until ipfs/go-ipfs-config#40 is merged

@Stebalien
Copy link
Member

Thanks! This is awesome!

What if we took the configuration in as a multiaddr? That is:

  • /ip4/1.1.1.1/udp/53 -> DNS
  • /ip4/1.1.1.1/tcp/443/https -> DNS over HTTPS
  • /ip4/1.1.1.1/tcp/443/https/HOST -> when we need a hostname.
  • /ip4/1.1.1.1/tcp/853/tls -> DNS over TLS (need to define the multiaddr protocol code for TLS but we need one anyways).

@Stebalien
Copy link
Member

That way, we can even specify a set of resolvers (to be tried in series).

@ItalyPaleAle
Copy link
Author

I actually like that idea! Would we just have a single configuration entry, then, something like "DNS" (as a string)? If that's empty, it would fallback to the system's resolver.

Does go-ipfs already have a multi-address parser built-in? If you could please give me a pointer to that, I'll look at making the update tonight (unless you want to make it yourself, you do have write access to my forks :) )

@Stebalien
Copy link
Member

something like "DNS" (as a string)? If that's empty, it would fallback to the system's resolver.

Just to be safe, I'd have:

"DNS": {
  "Resolver": /my/resolver...
  ...
}

We may want to make security requirements, caching, etc. configurable in the future.

@Stebalien
Copy link
Member

Does go-ipfs already have a multi-address parser built-in? If you could please give me a pointer to that, I'll look at making the update tonight (unless you want to make it yourself, you do have write access to my forks :) )

https://github.com/multiformats/go-multiaddr/

However, we need to quickly add a TLS protocol (we already need one anyways).

@Stebalien
Copy link
Member

PR to define TLS multicodec: multiformats/multicodec#145

@ItalyPaleAle
Copy link
Author

Doesn’t look like multiaddress would support something like this however?

/ip4/1.1.1.1/tcp/443/https/HOST

I could add the TLS protocol but I’m more worried about the host part?

To be frank, the Host part isn’t even strictly necessary:

  1. You could omit that, however then the client will not be able to validate the hostname in the TLS certificate
  2. You could use DNS-over-HTTPS by using the domain name rather than the IP. That will work, however there would need to be a DNS query first to resolve the ... resolver’s IP. And that query would go through the machine’s default resolver.

@Stebalien
Copy link
Member

I could add the TLS protocol but I’m more worried about the host part?

See multiformats/multiaddr#63 (comment). The current proposal is to define HTTP (which isn't really used in practice yet) as /http/HOST/path (given that HTTP 1.1 and on require this). When omitted, the HOST would be implied or omitted.

Looking around, it seems that the real issue is that js-libp2p has gone off and used the http protocol without a path.

...

I'll bring this up at the next core implementations meeting. For now, we can support /dns/cloudflare-dns.com/tcp/443/https while we get the multiaddr situation resolved once and for all.

@ItalyPaleAle
Copy link
Author

So, let me see if I understand correctly:

  1. For TLS:
    1. First, need to wait for add a TLS multicodec multiformats/multicodec#145 to be merged
    2. Next step is to add the TLS protocol to https://github.com/multiformats/go-multiaddr/
  2. For HTTPS: I'm confused to what the status is. Do we need to wait on Defining /http multiformats/multiaddr#63 ?

I won't be able to join the call next week, but I assume you'll want to discuss this PR :) If there are comments/feedback, we can take them offline here?

@Stebalien
Copy link
Member

I won't be able to join the call next week, but I assume you'll want to discuss this PR :) If there are comments/feedback, we can take them offline here?

We have a rule: it didn't happen if it isn't written somewhere on github. So yes, all resolutions and reasons will end up here.

Note: we'll likely schedule a second call during the first call to actually resolve the issue. I'll keep you in the loop in case you're interested (but it's only tangentially related to this issue).

@ItalyPaleAle
Copy link
Author

ItalyPaleAle commented Aug 8, 2019

Ok I pushed the code changes you requested:

  1. DNS resolver can now be set with a multi-address string "DNS.Resolver", using the format you proposed above. If it's empty, uses the system's resolver
  2. Two things are missing:
    1. Support for specifying a host after HTTPS, pending upstream
    2. Support for using the /tls protocol, pending upstream

As before, this depends on ipfs/go-ipfs-config#40 (which I just updated) and tests will fail until that's merged.

Does this design look good to you now? If so, I will start working on the unit tests tonight or tomorrow night

-- EDIT
Forgot to implement support for dns addresses. I'll add that too

@ItalyPaleAle
Copy link
Author

As I'm working on this, I noticed that some other parts of the codebase use a DNS resolver, calling the one inside https://github.com/multiformats/go-multiaddr-dns

I will need to update that too, likely

@ItalyPaleAle
Copy link
Author

I hit a road block while working on this during the weekend.

Turns out that https://github.com/multiformats/go-multiaddr-dns queries DNS too, and I believe that's used to resolve domain names inside multi-addresses in the config file (e.g. when the peers are defined with a DNS multi-address).

I could replace the DNS inside go-multiaddr-dns (either by changing the library or I believe at runtime too). The problem is that the DNS server to use is defined inside the .ipfs/config file, and I'm afraid of creating a bunch of spaghetti code because the config file is read after go-multiaddr-dns is loaded...

Thoughts?

@ItalyPaleAle
Copy link
Author

@Stebalien any update from today's call?

@Stebalien
Copy link
Member

Yes. Sorry for the delay. The conclusion was to ignore everything I've told you and to just manually configure all of this. We realized that, technically, we need to be able to specify a path for DNS-over-HTTPS and didn't feel like we'd be able to solve that issue in a single meeting.

I'm really sorry for the confusion, it just looks like multiaddr isn't quite ready for this use-case.

@Stebalien
Copy link
Member

So yeah. Let's have a Resolvers list with a list of objects describing each resolver. Later, when we finally get multiaddr support for this, we can replace this list with a list of multiaddrs (and do some json type-parsing magic to distinguish between the two).

@Stebalien
Copy link
Member

The real issue is that the HTTP multiaddr really needs to be a URL template (https://tools.ietf.org/html/rfc8484#section-4.1). Something like https://dnsserver.example.net/dns-query{?dns}.

Library: https://godoc.org/google.golang.org/api/googleapi/internal/uritemplates (?)

@ItalyPaleAle
Copy link
Author

Sorry for the radio silence on this, I have been very busy and I will be for at least another couple of weeks.

My biggest problem remains on how to reconcile the fact that go-multiaddr-dns needs a DNS resolver too.

go-multiaddr-dns is used while loading the config file of IPFS (in fact, it's what resolves the DNS names within the config file). So, when go-multiaddr-dns is loaded, there's no way to pass it the IPFS configuration to set a DNS server

The cleanest solution would be:

  1. Create a DNS resolver in a separate package.
  2. Defining the DNS configuration in a separate config file.
  3. Modify both go-multiaddr-dns and go-ipfs to use the DNS resolver from the package, and the separate config file.

Without that, I'm afraid we have a chicken-and-egg problem.

@matyapiro31
Copy link

How about adding test on dns_test.go?

return &DNSResolver{lookupTXT: net.LookupTXT}
func NewDNSResolver(cfg *config.Config) (*DNSResolver, error) {
// Check if we're using a custom DNS server
if len(cfg.DNS.Resolver) > 0 {

Choose a reason for hiding this comment

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

Please add system environmental variables.

@lidel
Copy link
Member

lidel commented Apr 22, 2021

Superseded by #8068 which takes bit different approach (go-multiaddr-dns) that works for both multiaddrs and dnslink (also, namesys got moved to https://github.com/ipfs/go-namesys).

See "Ongoing work" in #6532 for full context.

Thank you @ItalyPaleAle for taking the first stab at this and identifying relevant code paths! ❤️

@lidel lidel closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable DNS Servers
4 participants