[tor-bugs] #12535 [Pluggable transport]: goptlib should expose a SOCKS5 server instead of SOCKS4a.

[Tor Bug Tracker & Wiki]

#12535: goptlib should expose a SOCKS5 server instead of SOCKS4a.
-------------------------------------+----------------------------
     Reporter:  yawning              |      Owner:  yawning
         Type:  defect               |     Status:  needs_revision
     Priority:  normal               |  Milestone:
    Component:  Pluggable transport  |    Version:
   Resolution:                       |   Keywords:  goptlib, socks
Actual Points:                       |  Parent ID:  #12130
       Points:                       |
-------------------------------------+----------------------------

Comment (by yawning):

 Replying to [comment:7 dcf]:
 > Nice patch. It suffers somewhat from a lack of separation of
 concerns—not everything is directly related to SOCKS5 support. The biggest
 thing is the handling of concurrent connections, which is a new feature,
 and should be committed separately, if at all. Apart from that I just have
 some minor questions. I haven't looked at the connection logic yet nor the
 tests. See my comments below.

 Snipping stuff that's "ok, will fix".

 > {{{
 >  // Send a message to the proxy client that access to the given address
 is
 > -// granted. If the IP field inside addr is not an IPv4 address, the IP
 portion
 > -// of the response will be four zero bytes.
 > +// granted.  For interface backwards compatibility reasons, this does
 not set
 > +// BND.ADDR/BND.PORT correctly, however very few if any clients examine
 the
 > +// values of this field.
 >  func (conn *SocksConn) Grant(addr *net.TCPAddr) error {
 > -     return sendSocks4aResponseGranted(conn, addr)
 > +     // Addr in the SOCKS 4 code was the destination address, which is
 not sent
 > +     // in SOCKS 5.
 > +     return sendSocks5ResponseGranted(conn, nil)
 >  }
 > }}}
 >
 > I don't understand why addr is ignored? The comments don't make sense to
 me; isn't BND.ADDR/BND.PORT the destination address?

 Despite what certain things say (eg: Wikipedia), it's the local
 address/port of the outgoing socket from the SOCKS server to the
 destination.

 {{{
    In the reply to a CONNECT, BND.PORT contains the port number that the
    server assigned to connect to the target host, while BND.ADDR
    contains the associated IP address.  The supplied BND.ADDR is often
    different from the IP address that the client uses to reach the SOCKS
    server, since such servers are often multi-homed.
 }}}

 It's presumably done this way because clients can't call `getsockname()`
 on connections through a proxy, but in practice not many SOCKS clients
 require or use this information.  We could go and modify the application
 code that is affected by the switch to send back the right address as
 well.

 > The most troubling part of the patch is the new complex code surrounding
 AcceptSocks. First, thank you for taking the time to explore different
 ways of handling concurrent connections in a non-blocking way. However, if
 you leave that part out, the SOCKS5 code works just as well as the SOCKS4a
 code did, right? That is, it blocks a second connection while the first is
 still pending, but otherwise works correctly as long as SOCKS handshakes
 finish quickly? If so, then the new additional concurrency really needs to
 be in a separate patch. It's not intrinsically related to SOCKS5, and in
 fact you could make the same kind of enhancement to the SOCKS4a code as it
 stands.
 >
 > So I'd like to ask you to remove the new concurrency code and thereby
 simplify the patch. Even then, we should spend some more time thinking
 about the best way to implement the concurrency feature.
 > {{{
 >  type SocksListener struct {
 >       net.Listener
 > +     sync.Mutex
 > +
 > +     isClosed bool
 > +     ch       chan *SocksConn
 > +     wg       sync.WaitGroup
 >  }
 > }}}
 > A Mutex, a WaitGroup, a new boolean flag—there has to be a simpler way.
 If there's not a simpler way, maybe the new feature isn't worth it.
 Correctness is a virtue, but simplicity is also a virtue. In particular,
 you seem to be doing a lot of work to ensure that you can do a
 RejectReason to all concurrently connecting clients—I'm not sure that's
 worth the infrastructure required. We should aim for an implementation
 that does not require overloading the Close method in SocksListener.

 It's kind of complicated because on ln.Close(), the channel of pending
 connections needs to get flushed since there will be no further
 AcceptSocks() calls by the application code.  The "clean" thing to do is
 to fail the pending requests gracefully, though rudely closing off all the
 connections that are waiting pending Accept() is also an option.

 I'd be ok with removing the concurrency support, since the "incorrect"
 code will work well enough for our purposes (handshakes should get to the
 point where the command was received rather quickly).

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12535#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online