fix: bindIP not work on udp

[fregie]

64

udp associate not return a correct ip to client,cause Server.bindIP not used as localAddr when net.ListenUDP

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 65.39%. Comparing base (8de715f) to head (0ea4a8a). Report is 18 commits behind head on master.

Files	Patch %	Lines
handle.go	22.22%	6 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #63 +/- ## ========================================== + Coverage 63.13% 65.39% +2.25% ========================================== Files 14 14 Lines 784 705 -79 ========================================== - Hits 495 461 -34 + Misses 230 184 -46 - Partials 59 60 +1 ``` | [Flag](https://app.codecov.io/gh/things-go/go-socks5/pull/63/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=things-go) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/things-go/go-socks5/pull/63/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=things-go) | `65.39% <22.22%> (+2.25%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=things-go#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[thinkgos]

@fregie @ge9 which is the best implement? Give a idea about this, I do not use this library recently.

[fregie]

Mine. UDP Listener shouldn't use tcp listener's local addr.

[ge9]

Actually I'm not so confident about this :( In your code, what happens for requests from socks clients on other hosts? Does it choose correct external IP?

[fregie]

1. UDP local address may be different with TCP local address
2. In my implement,UDP local address will use the IP that assigned by WithBindIP in option.go.I'm not sure this will be the "correct" IP,but it must the one you want.

   // WithBindIP is used for bind or udp associate
   func WithBindIP(ip net.IP) Option {
   return func(s *Server) {
       if len(ip) != 0 {
           s.bindIP = make(net.IP, 0, len(ip))
           s.bindIP = append(s.bindIP, ip...)
       }
   }
   }

[ge9]

I agree that it is natural to use bindIP for UDP associate bind address. (I'm okay to revise my code like this) The problem is what IP address should be replied to the client. Assume go-socks5 is running at 192.168.1.1 and listening 0.0.0.0:10800, and the client on another host (192.168.1.2) is trying to use it via 192.168.1.1:10800. In your implementation, go-socks5 will use some port on 0.0.0.0 (e.g. 0.0.0.0:40000) for UDP associate and reply 0.0.0.0:40000 to the client, which surely does not work because we cannot connect to 0.0.0.0. Actually I tested your implementation with redsocks and it did not work. In this case, go-socks5 is running at 192.168.1.8 and redsocks is running at 192.168.1.12. 192.168.5.5 is another address of 192.168.1.12 and packets from this IP goes into TPROXY (redsocks).

10:36:56.820800 lo    In  IP 192.168.5.5.55272 > 3.132.228.249.3479: UDP, length 28
        0x0000:  4500 0038 0747 4000 4011 8543 c0a8 0505  E..8.G@.@..C....
        0x0010:  0384 e4f9 d7e8 0d97 0024 ae60 0001 0008  .........$.`....
        0x0020:  2112 a442 fe30 7c8e 6af1 22bd fe7f 0000  !..B.0|.j.".....
        0x0030:  0003 0004 0000 0000                      ........
10:36:56.820941 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [S], seq 4026329492, win 64240, options [mss 1460,sackOK,TS val 2177761205 ecr 0,nop,wscale 7], length 0
        0x0000:  4500 003c e60e 4000 4006 d148 c0a8 010c  E..<..@.@..H....
        0x0010:  c0a8 0108 8ec2 2a30 effc e994 0000 0000  ......*0........
        0x0020:  a002 faf0 8393 0000 0204 05b4 0402 080a  ................
        0x0030:  81cd ffb5 0000 0000 0103 0307            ............
10:36:56.821164 enp1s0 In  IP 192.168.1.8.10800 > 192.168.1.12.36546: Flags [S.], seq 1709076142, ack 4026329493, win 65160, options [mss 1460,sackOK,TS val 439202869 ecr 2177761205,nop,wscale 7], length 0
        0x0000:  4500 003c 0000 4000 4006 b757 c0a8 0108  E..<..@.@..W....
        0x0010:  c0a8 010c 2a30 8ec2 65de 6eae effc e995  ....*0..e.n.....
        0x0020:  a012 fe88 0f0a 0000 0204 05b4 0402 080a  ................
        0x0030:  1a2d b435 81cd ffb5 0103 0307            .-.5........
10:36:56.821212 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [.], ack 1, win 502, options [nop,nop,TS val 2177761205 ecr 439202869], length 0
        0x0000:  4500 0034 e60f 4000 4006 d14f c0a8 010c  E..4..@.@..O....
        0x0010:  c0a8 0108 8ec2 2a30 effc e995 65de 6eaf  ......*0....e.n.
        0x0020:  8010 01f6 838b 0000 0101 080a 81cd ffb5  ................
        0x0030:  1a2d b435                                .-.5
10:36:56.821259 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [P.], seq 1:4, ack 1, win 502, options [nop,nop,TS val 2177761205 ecr 439202869], length 3
        0x0000:  4500 0037 e610 4000 4006 d14b c0a8 010c  E..7..@.@..K....
        0x0010:  c0a8 0108 8ec2 2a30 effc e995 65de 6eaf  ......*0....e.n.
        0x0020:  8018 01f6 838e 0000 0101 080a 81cd ffb5  ................
        0x0030:  1a2d b435 0501 00                        .-.5...
10:36:56.821399 enp1s0 In  IP 192.168.1.8.10800 > 192.168.1.12.36546: Flags [.], ack 4, win 510, options [nop,nop,TS val 439202869 ecr 2177761205], length 0
        0x0000:  4500 0034 2068 4000 4006 96f7 c0a8 0108  E..4.h@.@.......
        0x0010:  c0a8 010c 2a30 8ec2 65de 6eaf effc e998  ....*0..e.n.....
        0x0020:  8010 01fe 3a5e 0000 0101 080a 1a2d b435  ....:^.......-.5
        0x0030:  81cd ffb5                                ....
10:36:56.821653 enp1s0 In  IP 192.168.1.8.10800 > 192.168.1.12.36546: Flags [P.], seq 1:3, ack 4, win 510, options [nop,nop,TS val 439202869 ecr 2177761205], length 2
        0x0000:  4500 0036 2069 4000 4006 96f4 c0a8 0108  E..6.i@.@.......
        0x0010:  c0a8 010c 2a30 8ec2 65de 6eaf effc e998  ....*0..e.n.....
        0x0020:  8018 01fe 3554 0000 0101 080a 1a2d b435  ....5T.......-.5
        0x0030:  81cd ffb5 0500                           ......
10:36:56.821674 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [.], ack 3, win 502, options [nop,nop,TS val 2177761206 ecr 439202869], length 0
        0x0000:  4500 0034 e611 4000 4006 d14d c0a8 010c  E..4..@.@..M....
        0x0010:  c0a8 0108 8ec2 2a30 effc e998 65de 6eb1  ......*0....e.n.
        0x0020:  8010 01f6 838b 0000 0101 080a 81cd ffb6  ................
        0x0030:  1a2d b435                                .-.5
10:36:56.821719 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [P.], seq 4:14, ack 3, win 502, options [nop,nop,TS val 2177761206 ecr 439202869], length 10
        0x0000:  4500 003e e612 4000 4006 d142 c0a8 010c  E..>..@.@..B....
        0x0010:  c0a8 0108 8ec2 2a30 effc e998 65de 6eb1  ......*0....e.n.
        0x0020:  8018 01f6 8395 0000 0101 080a 81cd ffb6  ................
        0x0030:  1a2d b435 0503 0001 0000 0000 0000       .-.5..........
10:36:56.822073 enp1s0 In  IP 192.168.1.8.10800 > 192.168.1.12.36546: Flags [P.], seq 3:25, ack 14, win 510, options [nop,nop,TS val 439202870 ecr 2177761206], length 22
        0x0000:  4500 004a 206a 4000 4006 96df c0a8 0108  E..J.j@.@.......
        0x0010:  c0a8 010c 2a30 8ec2 65de 6eb1 effc e9a2  ....*0..e.n.....
        0x0020:  8018 01fe 8198 0000 0101 080a 1a2d b436  .............-.6
        0x0030:  81cd ffb6 0500 0004 0000 0000 0000 0000  ................
        0x0040:  0000 0000 0000 0000 b395                 ..........
10:36:56.822141 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [F.], seq 14, ack 25, win 502, options [nop,nop,TS val 2177761206 ecr 439202870], length 0
        0x0000:  4500 0034 e613 4000 4006 d14b c0a8 010c  E..4..@.@..K....
        0x0010:  c0a8 0108 8ec2 2a30 effc e9a2 65de 6ec7  ......*0....e.n.
        0x0020:  8011 01f6 838b 0000 0101 080a 81cd ffb6  ................
        0x0030:  1a2d b436                                .-.6
10:36:56.822157 enp1s0 Out IP 192.168.1.12.36546 > 192.168.1.8.10800: Flags [R.], seq 15, ack 25, win 502, options [nop,nop,TS val 2177761206 ecr 439202870], length 0
        0x0000:  4500 0034 e614 4000 4006 d14a c0a8 010c  E..4..@.@..J....
        0x0010:  c0a8 0108 8ec2 2a30 effc e9a3 65de 6ec7  ......*0....e.n.
        0x0020:  8014 01f6 838b 0000 0101 080a 81cd ffb6  ................
        0x0030:  1a2d b436

The last part of the third last packet include "0500 0004 0000 0000 0000 0000 0000 0000 0000 0000 b395", which invalidly replies [::]:45973 instead of 192.168.1.8:45973. redsocks terminates connection due to the invalid address. The only clue to how go-socks5 is seen from the client is the local port of the original TCP connection. Hence my implementation.

[fregie]

Return the local addr that tcp listener used may work,but it is just a coincidence.What if the server is behind NAT? That will return a intranet address not working to client. So,it may work but it is not the right logic. Caller need to assgin the local addr that UDP listened and return to client,or we will never know the UDP ip to return to client in right logic. If caller doesn't assgin a bindIP or assgin a wrong one,it will not work,I think this is the right logic.

[ge9]

Yes, SOCKS5 behind NAT is rather serious problem. IMAO this is a fault of SOCKS5 protocol itself. But this is still a problem in your implementation. This problem can be solved on the client side. For example, a TPROXY-like software in Windows called Proxifyre seems to ignore the replied address and only cares the port.

Using TCP local address is not logically perfect but works in most situations. On the other hand, your implementation is at least incompatible with redsocks or hev-socks5-tproxy, which are most major ones of TPROXY client in Linux, even without any NAT.

[fregie]

No,you need to assign the right udp addr when you define the server options,and it will work well with all socks5 client. Like this:

opts := []socks5.Option{
        socks5.WithBindIP(udpip),
    }
    // Create a SOCKS5 server
    server := socks5.NewServer(opts...)
    if err := server.ListenAndServe("tcp", serveAddr); err != nil {
        log.Fatal(err)
    }

Configure the optiong wrong,and the it won't work.This is not a problem

[fregie]

Socks5 is a protocol.I don't think we should fix it incorrect to make it compatible with incorrect implementation of clients

[ge9]

Ah you are considering socks5 listens some external IP like 192.168.1.1 ? That will work, but it'll be more useful if it can listen all address and process clients from any address. At least, 3proxy seems to be implemented like this. My implementation works in almost all situations where yours works, except in very crazy settings such as NATting TCP and UDP differently.

[ge9]

I'm not saying altering SOCKS5 protocol. My implementation completely conforms to SOCKS5 specification and is compatible with more clients.

[ge9]

Rather, I suggest using a single "bind address" for both TCP and UDP. Allowing to set TCP listen IP and UDP listen IP differently is wierd, isn't it?

[fregie]

I understand what you mean.There is two conception we discussing. First is the UDP IP return to client. Second is the local addr to listen. In logic,this two thing is not the same,we should handle them separately. How about UDP listening on the same addr with tcp,and return the bindIP to client?

[ge9]

Hmm I'm afraid it still won't work if bindIP is 0.0.0.0 ? I think it's unnatural to return 0.0.0.0 to client.

[fregie]

I have told you,bindIP is only for assign udp ip for return to client If you assign a 0.0.0.0 to bindIP,what's wrong with you?

[ge9]

As I explained, returning 0.0.0.0 to client is problematic because the client cannot connect to 0.0.0.0, resulting in incompatibillity with redsocks or hev-socks5-tproxy.

[fregie]

Yeah,but that's a para error

[ge9]

I hope this software can listen all IPs at once and process clients accessing on different IPs. My implementation conform to SOCKS5 spec and no need to make it inconvenient.

[fregie]

You didn't get my point,I won't reply you any more. I stand by my opinion: UDP listening on the same addr with tcp or 0.0.0.0,and return the bindIP to client

[ge9]

Okay. Again, in my opinion, this is basically due to the fault of SOCKS5 proxy specification. However, at least, your implementation is incompatible with well-known socks proxy clients which are compatible to most of other well-known socks5 proxies , even in quite normal situations (without NAT). Using TCP local address will be a best choice in almost all cases. I hope the author will accept my PR.

[fregie]

Both implementation is not good enough for now,I don't care who's PR is accepted. But I think the plan should be what I said above,if it's need,I can't fix my code to my plan. @thinkgos You make the decision.

[ge9]

What I want to say is that, in your implementation, we cannot serve SOCKS5 on two extenal IPs at once, whatever bindIP you may choose. However, we may be able to serve two separate SOCKS5 on each IP. For example, if a machine has 192.168.0.1 and 10.0.0.1, we can set up two SOCKS5s at a single file (main.go), each of which uses 192.168.0.1 and 10.0.0.1. I was sticking to realize this by only one SOCKS5 binded to 0.0.0.0, but now I understood that configuring two SOCKS5 instances at main.go was not so bad (though some possible features like caching may prefer "sharing" bind IP). Okay, I'm fine for either PR, but I personally like mine :) (By the way, what is more interesting may be options to choose outgoing IP for each bindIP (or in my implementation, TCP local address).)

[ge9]

@fregie How about using TCP local addr only if bindIP is unspecified (or 0.0.0.0)? This behavior will be compatible with yours.

[ge9]

https://github.com/ginuerzh/gost/blob/fd57e80709aba9581757b1cd63b7d8f75e2385d2/socks.go#L1141 gost seems to listen the wildcard address and return TCP local address.

[ge9]

I sent a very similar PR (https://github.com/ginuerzh/gost/pull/1030) to gost and now it's merged (though go-socks doesn't have to obey gost).