-
Notifications
You must be signed in to change notification settings - Fork 103
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
fix(dns): allow http:// DoH resolvers #645
base: main
Are you sure you want to change the base?
Conversation
e99d6ba
to
f736895
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #645 +/- ##
==========================================
+ Coverage 60.55% 60.56% +0.01%
==========================================
Files 245 245
Lines 31126 31126
==========================================
+ Hits 18848 18853 +5
+ Misses 10607 10598 -9
- Partials 1671 1675 +4
|
@@ -15,8 +15,8 @@ var defaultResolvers = map[string]string{ | |||
} | |||
|
|||
func newResolver(url string, opts ...doh.Option) (madns.BasicResolver, error) { | |||
if !strings.HasPrefix(url, "https://") { | |||
return nil, fmt.Errorf("invalid resolver url: %s", url) | |||
if !strings.HasPrefix(url, "https://") && !strings.HasPrefix(url, "http://") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is going to be enough:
https://github.com/libp2p/go-doh-resolver/blob/f2e25860684789200e3a3a911dc9a74d01771d5b/resolver.go#L55
Note: AFAICT there is no DNS-over-HTTP spec for some reason it's specified as being over HTTPS, however I definitely see your point and have definitely felt the pain over the self-signed cert dance just to run local resolvers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, filled this PR because I am looking at https://github.com/libp2p/go-doh-resolver and https://github.com/multiformats/go-multiaddr-dns to wire up TTL (to fix #329 (comment) for DNSLinks), so likely will relax things there as well, and get back to this PR draft.
f736895
to
52cdf46
Compare
allows people to run own DoH resolver on the same box or within same secure VLAN/VPN/infra
52cdf46
to
3a62179
Compare
@@ -40,7 +40,7 @@ require ( | |||
github.com/ipld/go-codec-dagpb v1.6.0 | |||
github.com/ipld/go-ipld-prime v0.21.0 | |||
github.com/libp2p/go-buffer-pool v0.1.0 | |||
github.com/libp2p/go-doh-resolver v0.4.0 | |||
github.com/libp2p/go-doh-resolver v0.4.1-0.20241223184115-344e51cc8063 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Fix after libp2p/go-doh-resolver#28 is released
This PR works and for tools like CoreDNS that support DNS over HTTP (and not just HTTPS) this might be enough. If there's not enough support here folks might end up asking us for DNS over UDP / TCP support, but we can tackle that as the need arises. For what it's worth a nice thing about pushing on DNS-over-HTTP is that it makes it easier for folks who want to self-host while also using web-browsers since UDP/TCP are not available to javascript in browsers. For some cases like ENS this might not be a big deal since they can wrap the Ethereum JSON-RPC, but it might matter for other name systems. |
Updated the comment above given that CoreDNS does support DNS-over-HTTP in addition to DNS-over-HTTPS. FYI CoreDNS (and therefore the CoreDNS ENS resolver) supports DNS-over-HTTP, although the UX for this is that you configure listening on "https://" rather than "http://" and just not pass a certificate (which is what tripped me up initially 😅). I think we should be good to go on a merge here, pending reviews. |
This PR allows people to run own DNS over HTTP(S) resolver on the same box or within same secure LAN, without setting up unnecessary TLS certs.
Main use case is to simplify self-hosting of ENS resolves by gateway operations etc.
[ ] relax https://github.com/multiformats/go-multiaddr-dns (if needed)(doesn't appear to be needed)