[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:                       |
-------------------------------------+----------------------------
Changes (by dcf):

 * status:  needs_review => needs_revision


Comment:

 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.

 Please avoid rearranging function order and adding blank lines to existing
 functions. It's fine to make those kinds of changes directly on the trunk;
 they shouldn't show up in a feature branch.

 {{{
  // The package implements a SOCKS4a server sufficient for a Tor client
 transport
  // plugin.
  //
  // http://ftp.icm.edu.pl/packages/socks/socks4/SOCKS4.protocol
 }}}

 Put a URL to good SOCKS5 documentation in pt.go. I guess RFC 1928.

 {{{
 +       socksVersionString = "socks5"
 ...
 +// Returns "socks5", suitable to be included in a call to Cmethod.
 +func (ln *SocksListener) Version() string {
 +       return socksVersionString
 +}
 }}}

 Let's remove socksVersionString and put the magic string inside the
 function.

 {{{
 +       // the password string sent by the client.
 +       Password string
 }}}

 Capitalize the comment.

 {{{
  // 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?

 {{{
 -// Send a message to the proxy client that access was rejected or failed.
 +// Send a message to the proxy client that access was rejected or failed.
 This
 +// method exists for backwards compatibility and will only send the
 "General
 +// Failure" error code.
  func (conn *SocksConn) Reject() error {
 -       return sendSocks4aResponseRejected(conn)
 +       return conn.RejectReason(SocksRepGeneralFailure)
 +}
 +
 +// Send a message to the proxy client that access was rejected with the
 +// specified reason.
 +func (conn *SocksConn) RejectReason(reason byte) error {
 +       return sendSocks5ResponseRejected(conn, reason)
  }
 }}}

 Adding a RejectReason function is fine. It does tie us to SOCKS5 forever,
 but realistically we're not ever likely to change from SOCKS5. Don't
 characterize plain Reject as being for backward compatibility only; just
 document that it sends a general error; I don't think people should be
 required to use a specific code if they don't care.

 Here's related reading on the whole net.Error/err.Temporary thing (i.e.
 "Does this happen ever?").
 https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/commitdiff/3030f080eecf72b0e896236fca5fabd245c00bdb

 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.

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