-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Feature socks udp associate #250
base: master
Are you sure you want to change the base?
Conversation
impl is loosely based on https://github.com/armon/go-socks5 and https://github.com/haxii/socks5, but reworked rather heavily to support custom query handlers
…ver implementation instead of armon/go-socks5, add udp associate support
This looks awesome, especially considering the amount of changes required to pull this off. I will do some testing with it and report back. |
👍
…On Fri, 5 Mar 2021 at 4:50 pm aus ***@***.***> wrote:
This looks awesome, especially considering the amount of changes required
to pull this off. I will do some testing with it and report back.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#250 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE2X42NO57WJOEMXB5FW3DTCBWKRANCNFSM4YSAPGPA>
.
|
I tested it and works well. Please merge this asap 👍 |
Great addition, @Meteorite, that was exactly what I was looking for and it works like a charm! 🥇 @jpillora, can you merge this to the main Chisel repository? :-) |
hey @aus did you get a chance to look at this PR? my main concern is that we are now inheriting the complexity of those SOCKS modules, id prefer to first PR the required features into an existing maintained SOCKS module, and then we depend on that. @Meteorite i cant see the diff to the SOCKS modules - are there many changes? |
I did some testing a while back and I found no issues with the new SOCKS5 UDP ASSOCIATE feature. Valid point on inheriting the complexity. To level set, chisel currently uses armon/go-socks5@e753329 for its SOCKS implementation. A lot of the changes in this PR refect the original code in armon/go-socks5. @Meteorite based his code off of haxii/socks5 which added the UDP ASSOCIATE support. Here is a diff between the two: armon/go-socks5@master...haxii:master While this PR is bulky, it can be broken in to three chunks:
However, having attempted this implementation myself, these libraries don't work out of the box with chisel. There are some hoops to jump with typing and not having access to some private functions. So I guess our options are: a) forking haxii/socks5 into its own module, with necessary changes to support calling from chisel I guess the advantages of option A are that we reduce complexity of chisel's codebase, and we can blame a module if something breaks. 😄 I think option B is worth weighing considering how tightly coupled chisel is with the remote offerings nowadays. |
share/socks5 dir can be extracted as a library rather easily, if option A is to be taken. I can do it, if needed. But, indeed, coupling is rather tight, and some features in chisel might require socks5 subsystem modification. So, it might be more convenient to have its code in the same project. At least it was far more convenient during this feature development :) |
Thanks for reviewing @aus Given that socks is a fairly old protocol - my hope is there won't be changes. So I'm leaning towards option A. @Meteorite I might take you up on that offer, can we move 'share/socks' over to 'github.com/Meteorite/socks'? |
Ок, I'll do it. |
add dependency to github.com/meteorite/scope also
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.
Not enough time to fully review
Could you provide a summary of how SOCKS UDP works? I think I'm missing the forrest for the trees...
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= | ||
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= | ||
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= | ||
github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= |
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.
Could you please go mod tidy
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.
Already did it. testify
is here in go.sum
, because new dependencies (github.com/meteorite/scope
and github.com/meteorite/socks5
) use it for their tests:
> go mod graph | grep testify
github.com/meteorite/scope@v0.0.0-20210314203727-1e230fea59ae github.com/stretchr/testify@v1.7.0
github.com/meteorite/socks5@v0.0.0-20210604215257-bf325eecbc5d github.com/stretchr/testify@v1.7.0
...
so, it is a transitive dependency (needed to do go test all
). Note, it is not in go.mod
.
share/tunnel/socks_handler.go
Outdated
"net" | ||
) | ||
|
||
type chiselSocksHandler struct { |
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.
can we rename to socksHandler
?
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.
ok
share/tunnel/socks_handler.go
Outdated
udp *socks5.SingleUDPPortAssociate | ||
} | ||
|
||
func makeChiselSocksHandler(p *Proxy, localUDPAddr *net.UDPAddr, sl *log.Logger) *chiselSocksHandler { |
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.
can we rename to newSocksHandler
?
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.
ok
@@ -74,8 +82,19 @@ func (t *Tunnel) handleSSHChannel(ch ssh.NewChannel) { | |||
l.Debugf("Close %s%s", t.connStats.String(), errmsg) | |||
} | |||
|
|||
func (t *Tunnel) handleSocks(src io.ReadWriteCloser) error { | |||
return t.socksServer.ServeConn(cnet.NewRWCConn(src)) | |||
func (t *Tunnel) handleSocksTCP(l *cio.Logger, src io.ReadWriteCloser, hostPort string) error { |
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.
Interesting, so this function now reimplements ServeConn
?
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.
Yes, in some sense... It is an artifact of moving socks server from outgoing to incoming party and leaving only an outgoing traffic handler on outgoing party. Here is an outgoing handler. As we don't have socks server here, we should reimplement outgoing connection ourselves. Fortunately, it is rather simple.
share/tunnel/tunnel_in_proxy.go
Outdated
sl = log.New(os.Stdout, "[socks]", log.Ldate|log.Ltime) | ||
} | ||
p.socksServer, _ = socks5.New(&socks5.Config{ | ||
Handler: makeChiselSocksHandler(p, udpAddr, sl), |
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.
If this code does belong in Proxy, then Instead of using sl
we should use p.Logger.Fork("socks")
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.
Hmm.. The problem is Fork
produces cio.Logger
, that is chisel internal class. And this argument is here to be passed to socks5 library, which doesn't see chisel classes. So, we need standard log.Logger here and I reused this snippet:
Lines 60 to 64 in 92d90be
sl := log.New(ioutil.Discard, "", 0) | |
if t.Logger.Debug { | |
sl = log.New(os.Stdout, "[socks]", log.Ldate|log.Ltime) | |
} | |
t.socksServer, _ = socks5.New(&socks5.Config{Logger: sl}) |
To make it better, may be we should add a way to obtain stadard log.Logger from cio.Logger ? For now it is private there.
Ah just noticed the big summary in the original description sorry |
So this means, if I open port |
Yes, exactly. If another app needs to connect to chisel via firewall, then it can be a problem. But if we will force socks UDP port number to be equal to socks TCP port number, then chisel would fail with error on start if that UDP port is occupied even if user don't need socks UDP functionality. It seems, we need some cmdline way either to explicitly enable socks UDP (per remote or per all remotes) or to explicitly specify UDP port for socks remote. |
I will point out though that the “UDP firewall problem” is not specific to chisel or the implementation here. You would have the same problem in a standalone RFC-compliant SOCKS5 server. |
doesn’t this need to be a port range rather than a specific UDP port? it’s my understanding the UDP associate socks command works like this:
If we use a single port, how would the server keep track of the multiple clients or multiple destinations on a single UDP ASSOCIATE port? |
You are mostly right, but destination is specified on step 4, not step 2. On step 4 client must sent not an original UDP packet but UDP packet with a special UDP request header (see https://datatracker.ietf.org/doc/html/rfc1928, chapter 7). That header specifies the destination for this particular UDP packet. That is how server can track multiple destinations for the same client. On step 2 some destination might also be specified, but it is only kind of recommendation to server. RFC says:
Most socks servers just ignore this destination, received on step 2. |
So, to track multiple destinations and multiple clients server must remember source IP and port number of incoming packet from client and associate them with UDP stream. When backwards traffic from destination is received it must be forwarded to client's IP:port, from where original forward UDP traffic was received by server. That's why multiple clients can use the same socks server UDP port, but server will use their IPs and port numbers to distinguish between their UDP streams. If multiple clients want to send UDP packets to the same destination, server must send packets on their behalf from different ports to be able to distinguish backwards traffic. So, UDP socks server can be thought of as a kind of NAT. |
As I already said in PR description, github.com/meteorite/socks5 library has 2 strategies for UDP ports handling:
For now, SingleUDPPortAssociate is used in PR, that’s why, we might do it a little bit more firewall friendly, if we want. |
Single port per remote is enough for SingleUDPPortAssociate, but we can specify a range, if we want to switch to MultiUDPPortAssociate strategy or to allow searching for free port during initialization with SingleUDPPortAssociate. |
By the way, in this PR chisel creates one SSH channel per every UDP client app's IP:port pair of every socks remote. If client app's is sending UDP packets from the same IP:port to multiple destinations, then the same channel is reused for all such UDP streams. That's why each forward UDP packet is send via this channel with a header, indicating this packet destination, and each backward UDP packet is sent back via the channel with header, indicating remote party, from which this packet was received to transfer it to client app. |
Hey guys, thanks for the summary. I like where the intuitive use of where just works™ - and this is an area where things can silently fail, so if we can stick to single-port associate and not support multi-port then that's great. If we just do single-port, then socks server still need to be on the inbound side ( |
Yes, I think it is still far better to be on the inbound side, especially for forward remotes, as chisel server can work with multiple chisel clients simultaneously and each of chisel client can have multiple forward socks remotes with different UDP ports. If outbound side have to create UDP ASSOCIATE reply, then chisel server would need to know all those remotes' ports of all connected chisel clients and all connected chisel clients' IP addresses (as they must be reported to socks client app in UDP ASSOCIATE reply). Note, that now chisel server doesn't know chisel clients' inner IP addresses at all. And, may be even more important is that all that ASSOCIATE handshake would have to be transferred via SSH via WS from inbound to outbound side and back, if socks server is on the outbound side. To my mind, it is very inefficient comparing with replying from inbound side itself. And forward UDP packets (from client app to destination) filtering functionality would also be located on outbound side, that would require to transfer each UDP packet from inbound to outbound side even if this packet must be dropped. And, besides, having socks server on inbound side seems to be more flexible, as we could easily add some additional client traffic processing without additional passing inbound side's parameters to outbound side (i.e. without further chisel protocol changes). |
Hey @aus do you have time to review this? Unfortunately, I do not at this stage - though if you're happy to merge - feel free to do so, I'll tag a new release once it's in :) |
Gentle bump :) I don't know what's left to do, but it's working for me. Thanks for that awesome work @Meteorite |
@Meteorite I'm stealing your socks5 server to use in other projects :) many thanks. I'm going to benchmark against the incumbent but this definitely looks worth merging. |
Have yet to figure out a decent way to speed test UDP performance making this a little redundant but: proxychains iperf3 -N -c ping.online.net -p 5204 -P5 (via a chisel-udp-associate socks5 proxy to a machine in the same rack) [ ID] Interval Transfer Bitrate Retr proxychains iperf3 -N -c ping.online.net -p 5204 -P5 -R (via a chisel-udp-associate socks5 proxy to a machine in the same rack) [ ID] Interval Transfer Bitrate Retr proxychains iperf3 -N -c ping.online.net -p 5204 -P5 (via a chisel socks5 proxy to a machine in the same rack) [ ID] Interval Transfer Bitrate Retr proxychains iperf3 -N -c ping.online.net -p 5204 -P5 -R (via a chisel socks5 proxy to a machine in the same rack) [ ID] Interval Transfer Bitrate Retr I'm aware these benchmarks mean very little, really I should be running iperf3 on the chisel host machine... or doing this more than once. Obviously I'm yet to test the UDP bits, I'll just make a thing to fling packets over several ports and see what happens... nothing off the shelf I can find to do it. At a glance, for TCP it seems to perform well. For the record, from the host machine not going via chisel:
[SUM] 0.00-10.00 sec 267 MBytes 224 Mbits/sec 71 sender So not bad at all. Let me see if I can make something to test UDP assoc... the whole point of all this next. |
@ip-rw , you are welcome! :) |
i use this in another project and it works great :) |
Add UDP associate support to built-in socks proxy.
UDP associate protocol (see https://tools.ietf.org/html/rfc1928 , chapters 6 and 7) requires server to check client's UDP packets source IP address and drop packets from unknown IPs (i.e. from IPs, which don't hold active TCP connection to socks server). That's why it is very inconvenient and inefficient to implement this functionality in the current architecture with socks proxy on outgoing party (chisel server for forward socks or chisel client for reverse socks), as it will end up forwarding each packet from incoming party to outgoing via SSH may be only to drop it there. Besides, it would require chisel server to know incoming UDP port numbers of every connected chisel client to send proper replies to associate requests for forward socks.
Therefore, this PR moves socks server from outgoing party to incoming party, leaving only SSH-controlled connector on outgoing party. To do it we need a modular socks server implementation that allows injecting custom request handler and outgoing traffic handler. I failed to find such an implementation as a ready to use library and crafted custom socks server code, based on https://github.com/armon/go-socks5 and its fork with associate support https://github.com/haxii/socks5 . This crafted implementation is located in share/socks5 dir. When you create a socks server, you can pass a custom Handler implementation. Handlers could use one of associate implementation:
For now, SingleUDPPortAssociate is used in PR.
PR does not change any command line arguments to chisel client or chisel server. But one UDP port is opened for every socks remote on client and R:socks remote on server. Free UDP port number is chosen automatically (clients do not need to know it in advance, as they will receive it in replies to UDP associate requests to socks server).
Resolves #189