feat: support custom DoH resolvers #8068

vyzo · 2021-04-12T11:08:15Z

Closes #6532
Closes #6915
Closes #3942
Closes #2934

Depends on:

option for custom dns resolver libp2p/go-libp2p#1073 (merged; not released)
make DNS resolver pluggable go-namesys#10 (merged, not released)
add custom DNS Resolver configuration go-ipfs-config#126 (merged; not released)
refactor Resolver to support custom per-TLD resolvers multiformats/go-multiaddr-dns#26
Normalize domains to fqdn for resolver selection multiformats/go-multiaddr-dns#27
DoH resolver implementation libp2p/go-doh-resolver#1
Add DNS RR cache libp2p/go-doh-resolver#3

vyzo · 2021-04-12T11:14:34Z

seems like go-doh-resolver has an implicit dependency on go 1.16; fixing upstream package.

vyzo · 2021-04-12T11:27:26Z

squashed the gomod dep update commits.

aschmahmann

Overall looking good, left a few comments/questions.

There's also a DNS usage to update here https:/ipfs/go-ipfs/blob/ef866a1400b3b2861e5e8b6cc9edc8633b890a0a/core/commands/swarm.go#L491

core/node/dns.go

core/coreapi/coreapi.go

vyzo · 2021-04-12T18:45:26Z

I'll give it a rebase to resolve the conflicts in go.{mod,sum}

vyzo · 2021-04-12T18:50:18Z

gave it a rebase, resolved the conflict, and squashed a few commits.

aschmahmann · 2021-04-12T19:31:23Z

core/node/groups.go

@@ -281,6 +283,7 @@ func Online(bcfg *BuildCfg, cfg *config.Config) fx.Option {
 func Offline(cfg *config.Config) fx.Option {
 return fx.Options(
 fx.Provide(offline.Exchange),
+ fx.Provide(DNSResolver),


This preserves existing behavior. @Stebalien Is this the correct behavior, or would we want some nilDNSResolver here?

Yes, we should use the nilDNSResolver. I'd consider this a bug fix.

But we can also punt to a second PR.

It's pretty easy to do, so I can do it here.

undone, as it is a behaviour change that leads to a regression; we can easily consider it in a follow up pr.

core/node/dns.go

Stebalien · 2021-04-12T20:16:39Z

This needs some config documentation (config.md). Especially, how do we format DoH addresses?

Related: consider an upgrade path to DNS over TLS, and maybe even DNS over libp2p.

vyzo · 2021-04-12T20:24:45Z

sure, I'll add some documentation. Upgrade path could be very easy by using the appropriate url scheme -- eg `tls:` or `libp2p:`.

…

On Mon, Apr 12, 2021, 23:16 Steven Allen ***@***.***> wrote: This needs some config documentation (config.md). Especially, how do we format DoH addresses? Related: consider an upgrade path to DNS over TLS, and maybe even DNS over libp2p. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8068 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AAAI4SX6DETLPCLRG7BVBMTTINILNANCNFSM42ZCEYHA> .

lidel

We most likely will accept Multiadrs and URLs for DNS resolver endpoints,
but for now only https:// URL with DoH endpoint will be used in practice (Brave, ENS). Multiaddr support can be added later.
I requested config changes in add custom DNS Resolver configuration go-ipfs-config#126 (comment) / add custom DNS Resolver configuration go-ipfs-config#126 (review), it also includes example which we could re-use in docs.

core/node/dns.go

lidel

This is taking a nice shape 👍
I tweaked docs + left some inline comments regarding implicit defaults.
(I believe we should include implicit defaults in this PR, so we can then simply remove special-casing of eth in go-namesys and be done with it)

docs/config.md

core/node/dns.go

docs/config.md

lidel

Took this for a spin and works as expected 🚀
Only remaining ask is to add DNS record cache and make config bit more flexible (details below and inline)

DNS Cache

I think we need to add cache – DoH lookups are too expensive (~1 second each) for this to be useful for loading websites (each subresource would be slowed down by this). I filled multiformats/go-multiaddr-dns#28 for this, because we should have a global cache, and that seems like a good home for it.

Config

it should be possible to disable implicit default by setting value to "" (we need this, so people can run cleartext ENS resolver on localhost)
{} needs more work – details inline

docs/config.md

vyzo · 2021-04-14T17:02:47Z

DoH resolver cache implementation in libp2p/go-doh-resolver#3

vyzo · 2021-04-14T18:47:24Z

there is a failing sharness test, not sure if it is related:

130 - request for example.com/ipns/{fqdn} with X-Forwarded-Proto redirects to TLS-safe label in subdomain - sharnessLinux.t0114-gateway-subdomains

curl -H "Host: example.com" -H "X-Forwarded-Proto: https" -sD - "http://127.0.0.1:42979/ipns/en.wikipedia-on-ipfs.org/wiki" > response && test_should_contain "Location: https://en-wikipedia--on--ipfs-org.ipns.example.com/wiki" response

vyzo · 2021-04-15T09:26:42Z

Updated deps to go-libp2p@master and go-ipfs-config@master and squashed the gomod commits.

vyzo · 2021-04-15T22:08:47Z

Updated for the changes in the namesys constructor api.

…ame URL

- document implicit defaults for eth and crypto TLDs - be very honest and link to ToS of cloudflare-eth.com - provide suggestion that one should run own resolver if they really care about decentralization

License: MIT Signed-off-by: vyzo <[email protected]>

Stebalien · 2021-05-11T04:12:12Z

Updated to a released go-libp2p & go-ipfs-config.

addressed

vyzo requested review from lidel and aschmahmann April 12, 2021 11:08

vyzo force-pushed the feat/custom-resolver branch 2 times, most recently from 685bd0d to 45b9be5 Compare April 12, 2021 11:26

lidel mentioned this pull request Apr 12, 2021

Configurable DNS Servers #6532

Closed

9 tasks

aschmahmann requested changes Apr 12, 2021

View reviewed changes

core/node/dns.go Outdated Show resolved Hide resolved

core/coreapi/coreapi.go Outdated Show resolved Hide resolved

vyzo requested a review from aschmahmann April 12, 2021 18:42

vyzo force-pushed the feat/custom-resolver branch 2 times, most recently from e4537b2 to 40cbf8b Compare April 12, 2021 18:49

aschmahmann reviewed Apr 12, 2021

View reviewed changes

lidel requested changes Apr 13, 2021

View reviewed changes

core/node/dns.go Show resolved Hide resolved

vyzo requested review from lidel and aschmahmann April 13, 2021 14:52

lidel requested changes Apr 13, 2021

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

core/node/dns.go Show resolved Hide resolved

core/node/dns.go Outdated Show resolved Hide resolved

docs/config.md Outdated Show resolved Hide resolved

vyzo requested a review from lidel April 14, 2021 07:12

lidel requested changes Apr 14, 2021

View reviewed changes

docs/config.md Show resolved Hide resolved

vyzo requested a review from lidel April 14, 2021 16:04

vyzo force-pushed the feat/custom-resolver branch 2 times, most recently from fb908f6 to 66afda4 Compare April 14, 2021 16:08

vyzo force-pushed the feat/custom-resolver branch from 047ec45 to d78cb68 Compare April 15, 2021 09:26

lidel mentioned this pull request Apr 26, 2021

namesys: IPNS/DNS resolution is very slow #2934

Closed

aschmahmann added status/accepted This issue has been accepted status/blocked Unable to be worked further until needs are met and removed status/in-progress In progress labels Apr 27, 2021

vyzo and others added 16 commits May 10, 2021 21:03

gomod: deps, tidy

c9a0a03

explicit construction of DNS resolver

543c5da

fix dns test

aca3a20

create custom resolvers from config

b6a1ba9

update resolver config parsing, add eth. resolver by default

0ef2f8c

use NilDNSResolver for offline operation

531e9c1

update config documentation

5a1593e

the dot is not really an FQDN

b8eb1bb

docs: fix formatting and add links to DNS section

0a4defe

better handling of implicit defaults, reuse resolver object for the s…

77d6a08

…ame URL

use {} as default for DNS.Resolvers in config

9866c13

docs: clarify DNS.Resolvers

35fe392

- document implicit defaults for eth and crypto TLDs - be very honest and link to ToS of cloudflare-eth.com - provide suggestion that one should run own resolver if they really care about decentralization

allow overriding implicit defaults with the default resolver

0b4037c

License: MIT Signed-off-by: vyzo <[email protected]>

update for the new namesys api

eb7456d

undo dns neutering from offline mode

664c66c

chore: update deps

75e0141

Stebalien removed the status/blocked Unable to be worked further until needs are met label May 11, 2021

Stebalien force-pushed the feat/custom-resolver branch from b79689b to 75e0141 Compare May 11, 2021 04:11

test(sharness): fix test for updated address filters

3b254a6

Stebalien approved these changes May 11, 2021

View reviewed changes

Stebalien merged commit 4f4c947 into master May 11, 2021

Stebalien deleted the feat/custom-resolver branch May 11, 2021 05:10

aschmahmann mentioned this pull request May 14, 2021

Release v0.9.0 #8058

Closed

71 tasks

rvagg mentioned this pull request Jun 7, 2024

Update test files multiformats/multibase#125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support custom DoH resolvers #8068

feat: support custom DoH resolvers #8068

vyzo commented Apr 12, 2021 •

edited by lidel

Loading

vyzo commented Apr 12, 2021

vyzo commented Apr 12, 2021

aschmahmann left a comment

vyzo commented Apr 12, 2021

vyzo commented Apr 12, 2021

aschmahmann Apr 12, 2021

Stebalien Apr 12, 2021

Stebalien Apr 12, 2021

vyzo Apr 13, 2021

vyzo Apr 13, 2021

vyzo Apr 20, 2021

Stebalien commented Apr 12, 2021

vyzo commented Apr 12, 2021 via email

lidel left a comment •

edited

Loading

lidel left a comment

lidel left a comment •

edited

Loading

vyzo commented Apr 14, 2021

vyzo commented Apr 14, 2021

vyzo commented Apr 15, 2021

vyzo commented Apr 15, 2021

Stebalien commented May 11, 2021

feat: support custom DoH resolvers #8068

feat: support custom DoH resolvers #8068

Conversation

vyzo commented Apr 12, 2021 • edited by lidel Loading

vyzo commented Apr 12, 2021

vyzo commented Apr 12, 2021

aschmahmann left a comment

Choose a reason for hiding this comment

vyzo commented Apr 12, 2021

vyzo commented Apr 12, 2021

aschmahmann Apr 12, 2021

Choose a reason for hiding this comment

Stebalien Apr 12, 2021

Choose a reason for hiding this comment

Stebalien Apr 12, 2021

Choose a reason for hiding this comment

vyzo Apr 13, 2021

Choose a reason for hiding this comment

vyzo Apr 13, 2021

Choose a reason for hiding this comment

vyzo Apr 20, 2021

Choose a reason for hiding this comment

Stebalien commented Apr 12, 2021

vyzo commented Apr 12, 2021 via email

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

DNS Cache

Config

vyzo commented Apr 14, 2021

vyzo commented Apr 14, 2021

vyzo commented Apr 15, 2021

vyzo commented Apr 15, 2021

Stebalien commented May 11, 2021

vyzo commented Apr 12, 2021 •

edited by lidel

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading