[tor-bugs] #9221 [Obfsproxy]: obfsproxy does not have a SOCKS5 listener

[Tor Bug Tracker & Wiki]

#9221: obfsproxy does not have a SOCKS5 listener
---------------------------+------------------------------
     Reporter:  JBennett   |      Owner:  asn
         Type:  defect     |     Status:  needs_review
     Priority:  normal     |  Milestone:
    Component:  Obfsproxy  |    Version:  Obfsproxy: 0.1.4
   Resolution:             |   Keywords:
Actual Points:             |  Parent ID:
       Points:             |
---------------------------+------------------------------

Comment (by hellais):

 Here is a review of 0583a45.

 ## Proper error handling

 You should trap the errors inside of handleCmdConnectFailure by doing:

 failure.trap(error.NoRouteError, error.ConnectionRefusedError, etc.)

 Failure to do so will lead to the exception being re-raised.


 ## sendReply function

 I would put the code for constructing the reply corresponding to a
 successful
 request inside of SOCKSv5Outgoing.connectionMade rather than making it
 conditional inside of sendReply.

 Something like:

 ```
 def sendReply(self, reply, addr=0, port=0, atype=_SOCKS_ATYP_IP_V4):
     msg = _ByteBuffer()
     msg.add_uint8(_SOCKS_VERSION)
     msg.add_uint8(reply)
     msg.add_uint8(_SOCKS_RSV)
     msg.add_uint8(atype)
     msg.add(addr)
     msg.add_uint16(port, True)
     self.transport.write(str(msg))
 ```

 ## logging

 It is advisable to not call log directly but to wrap the calls to
 twisted.python.log by defining a log attribute inside of SOCKSv5Protocol.
 Then replace log.msg() with self.log() and call log.msg() inside of
 self.log.
 This makes it easier for a user of the protocol to disable logging.

 Also it seems like self.logging is not used anywhere. What is the purpose
 of it?

 ## Possible bugs

 I think at line 67 of socks5.py there is a typo. self.socks.send_reply
 should be self.socks.sendReply.

 ## Unittests

 The code should have unittests if there are plans to have it merged
 upstream
 into twisted. It's also probably a good idea to have them anyways.

 ## Class naming

 It's general not advisable to have two classes have the same name. You
 should
 probably change the names of the classes inside of socks.py to be
 OBFSSOCKSv5Outgoing, OBFSSOCKSv5Protocol and OBFSSOCKSv5Factory or
 something
 similar.

 Apart from this it looks like pretty good code, good job :)

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