mirror of
https://github.com/hickory-dns/hickory-dns.git
synced 2026-04-25 11:15:54 +03:00
[GH-ISSUE #1074] [resolver] custom socket connect calls #597
Labels
No labels
blocked
breaking-change
bug
bug:critical
bug:tests
cleanup
compliance
compliance
compliance
crate:all
crate:client
crate:native-tls
crate:proto
crate:recursor
crate:resolver
crate:resolver
crate:rustls
crate:server
crate:util
dependencies
docs
duplicate
easy
easy
enhance
enhance
enhance
feature:dns-over-https
feature:dns-over-quic
feature:dns-over-tls
feature:dnsssec
feature:global_lb
feature:mdns
feature:tsig
features:edns
has workaround
ops
perf
platform:WASM
platform:android
platform:fuchsia
platform:linux
platform:macos
platform:windows
pull-request
question
test
tools
tools
trust
unclear
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/hickory-dns#597
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @balboah on GitHub (Apr 14, 2020).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1074
I'm looking for a way to be able to customize how the connections are created in the https/tls and possibly later clear text resolvers (I need to bind it to a specific network interface).
What do you think if I try to implement a new dialer param like for example in https_client_stream,
which would then be checked for to be replacing the currently existing
let connect = Box::pin(TokioTcpStream::connect(name_server));Or would you reject this type of flexibility for trust-dns itself?
@balboah commented on GitHub (Apr 14, 2020):
some work in progress if you want to see what I'm heading towards:
@bluejekyll commented on GitHub (Apr 14, 2020):
Ah, this is great. I've been wanting to get to doing this for a while, so thank you for tackling the work.
While I see the direction your going it, I'm trying to decide between the impl your heading towards, vs. just a new bind address for the connection being passed in? I think it would be a little cheaper to pass the address (default would be
0.0.0.0). So I'm curious if you have other reasons to want to pass in the function pointer?Each connection type would benefit from having this ability, and I think as a generic configuration parameter too.
@balboah commented on GitHub (Apr 14, 2020):
Are you able to bind to a specific IP while doing tcp connect in rust without unsafe code?
One reason why I do it as a function pointer is so that I can keep the unsafe code to myself.
It would be simpler if it could pass a listener address config wise, as I can’t put this in config/options when it’s not serializable like the rest of the fields.
@bluejekyll commented on GitHub (Apr 14, 2020):
Yes. Here's where the connection is coming from: https://github.com/bluejekyll/trust-dns/blob/master/crates/proto/src/tcp/tcp_client_stream.rs#L142
It would require us to pass in a new socket addr for that creation with the bind. It looks like we might need to switch over to using this function for the TokioStream creation: https://docs.rs/tokio/0.2.18/tokio/net/struct.TcpStream.html#method.from_std
To bind you'd need to follow an example like this:
github.com/bluejekyll/trust-dns@3eaf3043d7/crates/proto/src/multicast/mdns_stream.rs (L183)Which uses
socket2library for configuring the socket. I hope that helps. This definitely feels like something we should make sure the Tokio networking modules help make this more straightforward.@balboah commented on GitHub (Apr 15, 2020):
Thanks for the detailed pointers, I'll take a stab at this later this week
@balboah commented on GitHub (Apr 17, 2020):
I just verified that:
works for implementing source port selections. However this will not work in my use case.
In my case I need to bind the socket to a specific interface or it won't work. Do you still think it makes sense to have an option for interface name or a callback function?
@bluejekyll commented on GitHub (Apr 17, 2020):
Could you explain why it won't work for your use case in a little more detail? For some reason I thought all you needed was the local bind address, but it sounds like you need more than that.
While my preference is to have just the bind address, I think we could do the bind function. I wonder though if the existing genericized Runtime stuff would be enough for your use case, or has the correct pattern. Right now we only have TCP and UDP abstracted here, but we can finish this abstraction up by adding TLS and HTTPS:
github.com/bluejekyll/trust-dns@224def9c92/crates/resolver/src/name_server/connection_provider.rs (L74-L86)Once that is in place, then you could define your own custom Runtime that would use a derivation of the TokioRuntime to perform the custom logic you need. You could even publish your own crate if you wanted, that would look like the AsyncStd crate: https://github.com/bluejekyll/trust-dns/tree/master/crates/async-std-resolver
To be clear, about all of this, the libraries are already fairly complex, and I would like to try and reduce that complexity. My concern with adding the callback is that it will add more complexity and indirection. So if we could avoid that, I think that's my preference. Though, I'm happy to be convinced otherwise.
@balboah commented on GitHub (Apr 20, 2020):
My use case is pretty narrow as I'm using the resolver on iOS, where I need to set it up so that the resolving happens through the VPN interface. Seems the only way to do that in native code is with the IP_BOUND_IF socket flag. When I ran a test on an iPhone to only use the
connect_bindthat I pasted earlier, the lookups would just stall.Using the RuntimeProvider trait is a great suggestion, I had missed this interface.
If I could use it to hook into the HttpsClientStreamBuilder when it creates a new TCP socket, it would be enough for my case. I don't think I necessarily need an "HTTPS" type as it's already using TCP as transport, unless that's how you want to abstract it. Might be confusing to mix transport layer and application layer types on this trait?
I saw that you just split out the async runtime crate recently, were you already considering implementing this for the TLS and HTTPS resolvers in the near term?
@balboah commented on GitHub (Apr 21, 2020):
Ah I see now why you need to provide a type for TLS as well in the trait. BTW, noticed that the futures AsyncRead/Write does not match the tokio ones.
oh right, that's why
AsyncIo02As03exists :)@bluejekyll commented on GitHub (Apr 21, 2020):
Sorry for the late response on this. This comment gave me pause:
Is it possible that we need to add a configuration option to iOS specifically for this? I see that you're already making a bunch of progress, so maybe we could start with something external initially, but it might be something we really do want to support directly in the library if it allows us to broaden the potential installation base to another platform. Just a thought.
But, yes, it looks like you've understood my suggestion properly and are making good progress.
@balboah commented on GitHub (Apr 22, 2020):
I think it will be OK to do it like you suggested with RuntimeProvider. Because it’s only when you run the resolver inside the VPN service process on iOS that this matters. A user who run the resolver in their non vpn app will use the correct route without having to think about it, even if a VPN is already established by a different app.
@balboah commented on GitHub (Apr 22, 2020):
one issue with this approach when you want to do something dynamic like when I'm not looking at how to toggle different interface names for the TCP connect, is that the Tokio TcpStream::connect() that we're mimicking is a static method. So I'll need to toggle different runtimes with a static interface name at compile time.
However I finally got it to work all the way in my project :)