Re: [tor-bugs] #9022 [Pluggable transport]: Create an XMPP pluggable transport

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#9022: Create an XMPP pluggable transport
---------------------------------+------------------------------------------
 Reporter:  asn                  |          Owner:  feynman 
     Type:  task                 |         Status:  accepted
 Priority:  normal               |      Milestone:          
Component:  Pluggable transport  |        Version:          
 Keywords:                       |         Parent:          
   Points:                       |   Actualpoints:          
---------------------------------+------------------------------------------

Comment(by asn):

 Replying to [comment:7 feynman]:
 > Replying to [comment:3 asn]:
 >
 > I should make a couple of notes here. First of all, the client really
 controls every aspect of initializing the connection. The xmpp bot on the
 server side just logs into an xmpp server and listens for traffic. It does
 not even bind to a port.
 >
 > On the client side, the xmpp bot binds to one or more ports and listens
 for traffic. It associates each of these ports with:
 > *An xmpp username to forward traffic to
 > *An ip:port that the xmpp bot on the server side should try to connect
 to
 >
 > When it gets a connection, it sends a message "connect me!" to the xmpp
 bot on the server through the chatline. It puts the ip:port the server
 should try to connect to in the xml subject tag and the ip:port of its
 newly spawned connect socket in the xml nick tag (used for nicknames).
 This way, the xmpp bot on the server side has a way to send data to that
 connected socket when replying. The client xmpp bot also creates an entry
 in its routing table that associates the following tuple:
 > (ip:port of connected socket, server's xmpp username, server ip:port to
 connect to)
 > with the newly connected socket.
 >
 > When the xmpp bot on the server side gets a connection request, it
 creates a new socket and tries to connect it to the ip:port specified in
 the subject tag. If successful, it adds an entry to its routing table that
 associates the following tuple:
 > (ip:port that the server just connected to, client's xmpp username,
 client's connected socket's ip:port)
 > with the newly connected socket.
 >
 > Now, when either socket receives data, they are prompted to send the
 data over the chat server using the socket's key in the routing table to
 construct the appropriate nick and subject xml tags. When a message is
 received over an xmpp server, the routing table key is constructed from
 the username of the computer that sent it, along with the nick and subject
 xml tags. The data is then forwarded to the appropriate socket.
 >
 > An analogous process takes place for disconnections, starting with a
 closing socket sending a "disconnect me!" message to the xmpp bot on the
 other side of the chat server.


 Hey there! I have a quick code review and comments. If you are bored
 fixing my comments, just say so, and I will do it when I get some free
 time.

 - All your git commit messages say 'master'. Instead commit messages are
 supposed to be a summary of what the commit does. Check out
 https://gitweb.torproject.org/tor.git for example.

 - Your comments are too verbose some times
 (https://github.com/aeftimia/hexchat/blob/master/fast/hexchat.py#L53
 https://github.com/aeftimia/hexchat/blob/master/fast/hexchat.py#L110
 https://github.com/aeftimia/hexchat/blob/master/fast/hexchat.py#L25),
 which makes the code hard to read, and some other times insufficient
 (https://github.com/aeftimia/hexchat/blob/master/fast/hexchat.py#L21 how
 do the keys/values of this dict look like?)
 (https://github.com/aeftimia/hexchat/blob/master/fast/hexchat.py#L123
 bottleneck?) If you want an example of a robustly commented Python
 codebase check out https://gitweb.torproject.org/stem.git or Twisted or
 something.

 - Also, check out http://www.python.org/dev/peps/pep-0008/ for Guido's
 ideas on Python comments.

 - Which XMPP plugins do we really need? For example, do we need Multi-User
 Chat?

 - Instead of using print(), use the Python logging module for your logs.

 - Stuff like "send '_' for empty data" must be mentioned in the spec.

 - Maybe add a more paranoid "(dis)connect me!" string so that it's even
 more unlikely to be encountered in a normal XMPP concersation? Add some
 numbers, and symbols, and stuff.

 - You are using rfind() but not checking the retval. You are also using
 functions like find() without checking for exceptions. Don't assume that
 the data you receive are correctly formatted. Your program might crash
 with an exception.

 - Try not to do catch-all excepts: http://ischenko.blogspot.gr/2005/01
 /exception-based-code-antipatterns.html

 - I still think sockbot is a weird name -- and you also needed two lines
 of comments to explain it. Why don't you name the class Hexchat or
 something?

 - What's the deal with the lambda in the "disconnected" event handler
 function pointer? Or the `lambda: False`? Am I missing something?

 - Maybe split the codebase to more files? One for the client and another
 for the server? This way you won't need to have variables like
 client_socks and server_socks that are only used in one mode.

 - Using `sock` as an abbreviation for `socket` continues to be confusing
 in names like `server_socks`. Maybe expand `sock` to `socket`?

 All in all, code looks good, the new comments are helpful, and I think I
 kind of understand how it works.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9022#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs