[proxy/vmess] Fix UDP over TCP fragmentation in VMESS + zero security

[Vigilans]

To make things easier, this PR is only scoped in VMESS zero security.

UDP proxying for VMESS zero is problematic, because VMESS zero does not attach any header to UDP-over-TCP traffic, causing source UDP packets to be fragmented into multiple packets, or merged into one packet.

Socks5 prefixed UDP packet with destination header, Trojan prefixed UDP packet with destination and length header. Considering packetaddr already does the job to prefix UDP packet with destination header, this PR implements a similar utility called packetstream, to make UDP packets correctly proxied on a stream by prefixing a length header.

When VMESS zero security is proxying UDP traffic, the PacketStreamWriter and PacketStreamReader will be used to correctly encode and recover UDP packets from the TCP stream.

This PR relies on https://github.com/v2fly/v2ray-core/pull/2336.

This PR is a draft, because I replaced the writer and reader globally for VMESS zero security, which is a breaking change for old servers and clients. I think it could be solved by introducing options or version number somewhere in the request header, but recently I don't have much time in proposing a mature solution, so I just enabled globally and adapted in both my clients and servers, and leave this draft for community to come up with a best compatible solution.

[codecov-commenter]

Codecov Report

Base: 39.21% // Head: 39.16% // Decreases project coverage by -0.05% :warning:

Coverage data is based on head (843c248) compared to base (83ea1ba). Patch coverage: 8.62% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2337      +/-   ##
==========================================
- Coverage   39.21%   39.16%   -0.05%     
==========================================
  Files         617      618       +1     
  Lines       36859    36910      +51     
==========================================
+ Hits        14454    14457       +3     
- Misses      20816    20864      +48     
  Partials     1589     1589              

Impacted Files	Coverage Δ
common/net/packetstream/packetstream.go	0.00% <0.00%> (ø)
proxy/vmess/encoding/client.go	40.30% <0.00%> (-0.95%)	:arrow_down:
proxy/vmess/encoding/server.go	26.39% <0.00%> (-0.51%)	:arrow_down:
common/buf/buffer.go	80.46% <33.33%> (-6.61%)	:arrow_down:
common/log/logger.go	82.35% <0.00%> (+1.47%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[Vigilans]

Since this is a draft PR introducing packetstream, I'd leave here some draft thoughts on packetaddr.

I think usage of packetaddr in inbound should be moved to dispatcher. This is necessary for that:

1. for packetaddr to not break sniffer. UdpPacketAddrDispatcher actually reduces dispatched linkes to one per (src, dst) to one per (src). This makes sniffer on dispatcher sees all packets from one source to different destinations in one link, causing the sniffer to not work at all.
2. for packetaddr to be implemented for dokodemo door. UDP connections for other inbounds merely based on (src), but for dokodemo door it's based on (src, dst). That means udp links are already split before entering the inbound Process() routine, using UdpPacketAddrDispatcher will not work.

By processing packetaddr logic in dispatcher, we can:

1. Multiplex udp packets of same (src) on one link, but cache the payloads to different destination to different buffers, and call sniffer on each buffer, so sniffer is still (src, dst) based.
2. Even if for dokodemo door Process() is called for each (src, dst) tuple, each Process() call would receive a same link from dispatcher if their (src) is the same.
3. We could directly specify fullcone: true | false in Inbound settings, instead of writing once for each proxy settings. The fullcone: true works like sniffingRequest, that dispatcher will see this settings in context, then enable it for the inbound.

Above thoughts requires a major refactoring of dispatcher app, which is exactly what I've done after prior DNS updates. By taking packetaddr into account a more mature and powerful dispatcher / sniffer / router app can be designed. However since I am busy these times, I do not have much time formalizing my proposals and tests, to file a PR, so just leave drafts here for reference.

[xiaokangwang]

Firstly, thanks for your work on this. I will review and merge its dependent PR first and have a look if there is a nice way to improve this pull request.

As for your suggestion on moving the packet addr too dispatcher, I would like to have more discussion before advise you to proceed:

1. If we wants dokodemo to receive all traffics from the same (src), there is one line of code we could change: https://github.com/v2fly/v2ray-core/blob/83ea1ba3ba94c944d99cb063bc6c5f42d6cf2fff/app/proxyman/inbound/worker.go#L286 . We shouldn't split the traffic one place and reassemble them somewhere else as this creates two issues: A: when the traffic is being reassembled at dispatcher, traffic from different inbound may have the same (src) and get assembled into same stream. B: dispatcher will need to have a global map that requires read and write access from all udp connection dispatch, creating lock contention.
2. sniffing don't work by design. If a user wants to sniff, then iptables should be used to direct traffic to different inbounds.

[rurirei]

sniffing don't work by design.

what about implement packetaddr (to cache (or read out and write back) the addr header) over dispatcher.cachedReader?

[AkinoKaede]

I recommend merging this PR, although it may be broken, this feature is not working properly.

[github-actions[bot]]

It has been open 120 days with no activity. Remove stale label or comment or this will be closed in 5 days

[github-actions[bot]]

It has been open 120 days with no activity. Remove stale label or comment or this will be closed in 5 days