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

Implement remote DNS #374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

blechschmidt
Copy link

Hi,

since this feature has been requested multiple times, I am making another attempt to implement the remote DNS feature.

Remote DNS intercepts UDP DNS queries for A records on port 53. It replies with an unused IP address from an address pool, 198.18.0.0/15 by default. When obtaining a new address from the pool, tun2socks needs to memorize which name the address belongs to, so that when a client connects to the address, it can instruct the proxy to connect to the FQDN. To implement this IP to name mapping, ttlcache is used. To prevent using multiple addresses for the same name, ttlcache is also used to implement a name to IP mapping. If an IP address is already cached for a name, that address is returned instread. When building a connection, the connection metadata is inspected and if the destination address is associated with a DNS name, the proxy is instructed to use this name instead of the IP address.

@blechschmidt blechschmidt force-pushed the merge branch 2 times, most recently from 427e949 to 1c2355c Compare July 15, 2024 21:02
@xjasonlyu
Copy link
Owner

xjasonlyu commented Jul 15, 2024

Hi there, first of all, thanks for your PR (again)!

Right, the remote DNS feature has been requested many times, and I actually had a draft proposal code for it somewhere in my git stash months ago. ;-)

My proposal design for it was mostly similar, but with some differences:

  1. I personally think it's better to have a FakeDNS component listening on port 53 than to intercept/hijack all DNS requests, so that users can choose to set the DNS themselves and make it less invasive.
  2. I personally would use this fakeip module to implement a FakeDNS that was endorsed/tested by many users and seemed to be more robust and stable.
  3. The TTL for each response should be set to 1s. This is a common practice used in many fake dns/ip related tools to mitigate the fake responses to the system cache.

Anyway, it would be nice if we could finally get it to work. Let me know if you have any other opinions before proceeding to the next step.

@blechschmidt blechschmidt force-pushed the merge branch 5 times, most recently from 6a0dc7e to 4e5d962 Compare July 16, 2024 00:32
@blechschmidt
Copy link
Author

Hi,

thanks again for your feedback.

  1. I personally think it's better to have a FakeDNS component listening on port 53 than to intercept/hijack all DNS requests, so that users can choose to set the DNS themselves and make it less invasive.

    Does the following change sufficiently address this?

    isCorrectEndpoint := id.LocalPort == 53 && (listenAddress.Equal(id.LocalAddress.AsSlice()) || listenAddress.IsUnspecified())
    // Ignore UDP packets that are not matching the listen address and are not recursive queries
    if !isCorrectEndpoint || err != nil || len(msg.Question) != 1 || msg.Question[0].Qtype != dns.TypeA &&
    msg.Question[0].Qtype != dns.TypeAAAA || msg.Question[0].Qclass != dns.ClassINET || !msg.RecursionDesired ||
    msg.Response {
    return false
    }

    A listen address (which also supports unspecified addresses) can now specified through the command line here:
    flag.StringVar(&key.RemoteDNSListenAddress, "remote-dns-listen-addr", "198.18.0.1", "IP to listen on for DNS requests")

  2. I had a look at Clash's fakeip module but I did not want to introduce all the abstraction layers, such as DNS middleware, filters etc., that came with it. Regarding the logic, i.e. cycling through IP addresses until there is vacant cache key, this implementation does essentially the same. (Unless there is no other other bug, of course. I have just fixed one in 6a0dc7efb850b4c6155196c9ec7906320ecb0005.) Can we maybe agree on getting this feature merged without all the abstraction layers as is and maybe introduce the implementation based on fakeip later, if still desired? I guess most users only need a really basic feature allowing the resolution to happen by the proxy.

  3. I don't think that a TTL of 1s as implemented by Clash is necessarily the best choice. With 1s, a website that loads a few resources from the same domain can result in many unnecessary DNS lookups. But it's a trade-off to eliminate caching issues after stopping the tool and I don't want this to be a blocker, so I have changed the default to 1 here:

    flag.DurationVar(&key.RemoteDNSTTL, "remote-dns-ttl", 1*time.Second, "TTL for remote DNS")

@xjasonlyu
Copy link
Owner

Thanks for the quick update.

  1. For this FakeDNS component listening on port 53, I meant to make it as a separate DNS service/server listening on the system stack. There are several reasons for this: a) gVisor updates frequently, so we'd prefer to decouple it as much as possible. b) performance is limited if we do this process outside the system stack, which can increase cpu usage. (it's unnecessary to do it within the userspace stack)
  2. That's what I was actually trying to say. We don't need any of the abstract layers from clash, just the fakeip module itself, which generates all the fake ips and maintains the ip pool. But one thing we might need, if we want it to be a real DNS server, is to adapt some fake DNS server logic from it. But again, yeah, we don't need abstract layers, I personally want to keep the component and the whole server logic as simple as possible.
  3. Personally, I wouldn't worry about the multiple DNS lookups, especially if we implement the fake DNS as a real server, the overhead would be too small to consider (We have LRU cache for each lookup and the UDP DNS query is small and quick). This 1s TTL approach is not only used by clash, but also by other proxy/network tools like v2ray, surge, etc. So I think it's mature enough to be used even in production.

So ideally the core package should remain unchanged and we will introduce a new fakeip package which will be used to implement a fake dns server under the dns pkg. Then only a few minor adjustments would be made under, e.g. tunnel, engine, metadata pkg.

@blechschmidt blechschmidt force-pushed the merge branch 2 times, most recently from 90e1c72 to 14eaa35 Compare July 19, 2024 13:51
@blechschmidt
Copy link
Author

Thanks for your suggestions.

  1. For this FakeDNS component listening on port 53, I meant to make it as a separate DNS service/server listening on the system stack. There are several reasons for this: a) gVisor updates frequently, so we'd prefer to decouple it as much as possible. b) performance is limited if we do this process outside the system stack, which can increase cpu usage. (it's unnecessary to do it within the userspace stack)

Regarding a), you are right that changes in gVisor could impact this implementation. Concerning b), I think the performance impact is negligble given the amount of requests that one would expect under normal operation.

However, I have implemented it that way now. By default, a listener is started on 127.0.0.1:53. A drawback of this implementation is that the OS actually needs an interface with the listener address. Further, it may interfere with software that is already occupying port 53. I am thinking of systemd-resolved here.

  1. That's what I was actually trying to say. We don't need any of the abstract layers from clash, just the fakeip module itself, which generates all the fake ips and maintains the ip pool. But one thing we might need, if we want it to be a real DNS server, is to adapt some fake DNS server logic from it. But again, yeah, we don't need abstract layers, I personally want to keep the component and the whole server logic as simple as possible.

I have copied the fakeip module and dependencies to tun2socks in the last changes. It now uses that module. IMO, it is still more complex than necessary, but as you say, it has been proven to work.

  1. Personally, I wouldn't worry about the multiple DNS lookups, especially if we implement the fake DNS as a real server, the overhead would be too small to consider (We have LRU cache for each lookup and the UDP DNS query is small and quick). This 1s TTL approach is not only used by clash, but also by other proxy/network tools like v2ray, surge, etc. So I think it's mature enough to be used even in production.

1s is now the default (and there is no option to change it).

So ideally the core package should remain unchanged and we will introduce a new fakeip package which will be used to implement a fake dns server under the dns pkg. Then only a few minor adjustments would be made under, e.g. tunnel, engine, metadata pkg.

This is the case now. The dns pkg implements a simple DNS server with a very limited logic compared to the clash implementation. However, it uses the fakeip package which has simply been copied from clash.

Could you review the latest changes and provide feedback on whether the latest changes are going into the right direction?

Thanks a lot for your work.

This commit implements fake DNS.

Fake DNS implements a UDP listener DNS A record queries on port 53. It
replies with an unused IP address from an address pool, 198.18.0.0/15 by
default. When obtaining a new address from the pool, tun2socks needs to
memorize which name the address belongs to, so that when a client
connects to the address, it can instruct the proxy to connect to the
FQDN. To implement this IP to name mapping, the FakeIP module from clash
is used.
@xjasonlyu
Copy link
Owner

Thank you so much for your incredible work, and I think it's overall on the right track!

Regarding a), you are right that changes in gVisor could impact this implementation. Concerning b), I think the performance impact is negligble given the amount of requests that one would expect under normal operation.

However, I have implemented it that way now. By default, a listener is started on 127.0.0.1:53. A drawback of this implementation is that the OS actually needs an interface with the listener address. Further, it may interfere with software that is already occupying port 53. I am thinking of systemd-resolved here.

Right, I understand your concern. But I think for most users who would like to enable this fake dns service, they should be able to take care of services like systemd-resolved on their own end.

And I was thinking that maybe we can add an option sometime, for example fakedns-hijack, which intercepts the DNS packets in the tunnel/udp part and returns the fake DNS response (in the future, but it should be good for now).

I have copied the fakeip module and dependencies to tun2socks in the last changes. It now uses that module. IMO, it is still more complex than necessary, but as you say, it has been proven to work.

Yep, I agree that clash's fakeip is a bit complex. Not sure if we can try to strip some unnecessary features from this package to minimize and simplify it... 🤔

1s is now the default (and there is no option to change it).

Thanks. It should be fine then. 👍

@xjasonlyu
Copy link
Owner

FYI, I may need some time to review and test all the features, and I apologize in advance if it takes a little longer as I am currently busy with my personal errands ;-)

In the meantime, please feel free to update or provide any suggestions that might be helpful. Thanks again.

@github-actions github-actions bot added the Stale label Sep 18, 2024
@github-actions github-actions bot closed this Sep 25, 2024
@xjasonlyu xjasonlyu added enhancement New feature or request and removed Stale labels Sep 29, 2024
@xjasonlyu xjasonlyu reopened this Sep 29, 2024
@github-actions github-actions bot added the Stale label Nov 29, 2024
@github-actions github-actions bot closed this Dec 6, 2024
@xjasonlyu xjasonlyu added pending Waiting for further review and removed Stale labels Dec 20, 2024
@xjasonlyu xjasonlyu reopened this Dec 20, 2024
@xjasonlyu xjasonlyu self-requested a review December 20, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pending Waiting for further review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants