feat(transport): add DNS-over-TCP PacketListener

[jyyi1]

dnsovertcp.NewPacketListener(sd) can be used to handle UDP based DNS requests when the remote Shadowsocks server does not support UDP traffic. This is done by using the StreamDialer sd to query DNS request over the TCP transport as decribed in RFC 1035.

Unlike the dnstruncated.NewPacketListener introduced in #26 , we won't depend on the DNS client implementation, because we will do the DNS-over-TCP request on behalf of the DNS client.

Related PR: #26

[fortuna]

I made some suggestions to improve the implementation, but given the change in performance characteristics, we need to understand what we are getting out of it to justify this change.

[fortuna]

Suggested change

	// length field. This length field allows the low-level processing to assemble a complete
	// message before beginning to parse it.
	// length field. This length field allows the low-level processing to allocate a complete
	// message before beginning to parse it.

[fortuna]

I believe there's a much simpler implementation.
ReadFrom could simply read from a channel that returns a (buffer, error).

Each DNS request will start a Go routine that:

• Dials the DNS resolver
• Writes the request
• CloseWrite
• Read the response
• CloseRead
• Write response to buffer

Then the ReadFrom could be:

• Check the deadline and return accordingly
• if deadline in the future, wait on the channel and return that

SetDeadline can schedule a timer to write a deadline exceeded error to the channel. This will force return a pending ReadFrom and future reads will fail until the deadline is reset (because of the deadline check).

This is not thread safe, but you can protect the method implementation with a Mutex to guarantee sequencing.

You may want to add all TCP connections to a list to close when the PacketConn is closed.

[fortuna]

Could you clarify your goal with this approach, versus the one we have been using?

I worry that this approach changes the performances characteristics of the client, without clear benefit.

This approach makes the cases where TCP is needed worse, because you need two sequential TCP connections: the first one will return a response too big for UDP, and the OS/Client will have to start a second TCP connection, doubling the time. It's not clear to me how often this happens. Likely not often. But it may be an issue with DNSSEC or ECH/HTTPS record.

Currently, the client will immediately be notified to try TCP, resulting in a single TCP connection.

Furthermore, we leave it to the client to handle the management of the TCP connections. They may have optimizations like reuse of connections. And if frees us from the responsibility of doing NAT for UDP.

In other words, we don't need to maintain the complexity of this implementation if we just return the truncated bit.

That also means the OS/Client will be handling the resources for all the connection tracking.

Another concern is the blocking of WriteTo on a TCP connection, which wasn't blocking before.

Given the downside, if there isn't a good reason for changing our approach, I suggest we go with the original approach.

[jyyi1]

Here are the two call sequences. Basically in this PacketListener, we don't need to depend on DNS client's retry logic, we will do the DNS-over-TCP for them.

Old DNS Fallback PacketListener (See #26)

Sequence:

• (1): DNS request over UDP
• (2): In WriteTo, send the request to ReadFrom through an internal channel
• (3): In ReadFrom, set the "TC" (truncated) bit (see feat(network): add DNS fallback (truncated) PacketProxy #26) and return
• (4): DNS client retry it over TCP (now it will use the shadowsocks.StreamDialer to read/write the TCP traffic)

The DNS Client needs to implement the retry-over-TCP logic when it sees the "TC" (truncated) bit set in the UDP response.

    DNS Client (e.g. browser)  <--(4)-->  shadowsocks.StreamDialer  <->  DNS Resolver (e.g. 8.8.8.8)
             |        ^
             |        |
            (1)       +-----------------(3)------------------+
             |                                               |
             v                                               |
dnstruncate.PacketListener.WriteTo(...)   ---(2)-->  dnstruncate.PL.ReadFrom(...)

This PacketListener (DNS-over-TCP)

Sequence:

• (1): DNS request over UDP
• (2): In WriteTo, wrap the DNS request in a TCP packet, and send the request-over-TCP by using shadowsocks.StreamDialer
• (3): The shadowsocks.StreamDialer will communicate with the actual DNS resolver over TCP and get the response (in TCP)
• (4): In ReadFrom, read the DNS response (in TCP) from the tcp connection, and convert it to a UDP packet
• (5): Return the DNS response (in UDP) back to the DNS Client

The DNS Client doesn't even need to know anything behind the scene, from its perspective, the UDP DNS requests successfully responded with a UDP DNS response.

    DNS Client (e.g. browser)
             |        ^
             |        |
            (1)       +-----------------(5)------------------+
             |                                               |
             v                                               |
dnsovertcp.PacketListener.WriteTo(...)             dnstovertcp.PL.ReadFrom(...)
                                |                                   ^
                                |                                   |
                               (2)              +--------(4)--------+
                                |               |
                                v               |
                              shadowsocks.StreamDialer  <--(3)-->  DNS Resolver (e.g. 8.8.8.8)

[fortuna]

I'm not worried about the client, because the client here is the operating system, which knows how to handle DNS properly.

Implementing a DNS client is complicated, so we can leverage the fact that the OS already provides the implementation. For instance, your client doesn't consider EDNS0, which allows the client to specify a larger buffer size.

I found some data on the truncation bit:

From Fragmentation, truncation, and timeouts: are large DNS messages falling to bits? (blog post).

Around 3-7% of DNS responses had the TC bit set. However, that considers the use of EDNS0. Without it, the retries would happen a lot more. It looks like 40% of more of the UDP responses were larger than 512 bytes:

By implementing the DNS-over-TCP logic we are forcing two double TCP connections (2 x (client-proxy + proxy-resolver)), effectively 8 round trips instead of 4 (2 for UDP), for 40% of the resolutions, which is substantial.

Given the added complexity, and performance regression, I don't think us and others will use this logic.

[fortuna]

I don't think this is the appropriate errors for most UDP packets that are received. This is what we currently do: https://github.com/eycorsican/go-tun2socks/blob/301549c435ff1ca8fc353bee3763ae639c727b89/proxy/dnsfallback/udp.go#L31

Perhaps EPROTONOSUPPORT or EPROTOTYPE would be more appropriate.

[fortuna]

This waits on a TCP connection and will cause head-of-line blocking. You won't be able to process the next UDP packet until this write returns, while you could be doing that in parallel.

Move the Dial+Write to the Go routine. You already have the buf in listenDNSResponseOverTCP. Copy p there before starting the go-routine, and use it for the Write to the proxy.

[fortuna]

Here is another critical architecture choice: this logic should probably be in a lwip2transport UDP handler.
We may want to define a type for that.

That allows us to return an error on the Connect call, which we can't with this approach. I believe that may have implications on what the lwip sends to the TUN device (ICMP error?)

Implementing the handler also removes the need to implement a PacketConn, which makes the logic more complicated.