[tor-bugs] #6951 [Stem]: Implement a wrapper method for MAPADDRESS requests

[Tor Bug Tracker & Wiki]

#6951: Implement a wrapper method for MAPADDRESS requests
--------------------+-------------------------------------------------------
 Reporter:  neena   |          Owner:  neena         
     Type:  task    |         Status:  needs_revision
 Priority:  normal  |      Milestone:                
Component:  Stem    |        Version:                
 Keywords:          |         Parent:                
   Points:          |   Actualpoints:                
--------------------+-------------------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 Hi Ravi. Looks great!

 > response = self.msg("MAPADDRESS %s" % " ".join([k + "=" + mapping[k] for
 k in mapping.iterkeys()]))

 Maybe tiny bit better as the following?

 {{{
 mapaddress_arg = " ".join(["%s=%s" % (k, v) for (k, v) in
 mapping.items()])
 response = self.msg("MAPADDRESS %s" % mapaddress_arg)
 }}}

 > -    Checks if all of our lines have a 250 response.
 > +    Checks if any of our lines have a 250 response.

 Why are you changing this? If we have an error response mixed in then why
 would the message be ok? If MAPADDRESS provides mixed response codes then
 maybe it's the exception?

 > +        try: key, value = message.split("=", 1)
 > +        except ValueError: raise stem.socket.ProtocolError(None, "Not a
 mapping")

 Lets include the message in the exception so we know what isn't a mapping.
 Maybe something like...

 {{{
 if '=' in message:
   key, value = message.split("=", 1)
   self.entries[key] = value
 else:
   raise stem.socket.ProtocolError(None, "MAPADDRESS returned '%s', which
 isn't a mapping" % message)
 }}}

 > - Changed "SocksPort 0" to "SocksPort 29327" from the base torrc. (29327
 is random)

 The control port that we use for the integ tests is 2779. Maybe pick
 something next to that so the port range that our tests rely on is
 contiguous? It's easier to say "stem's integ tests need ports X-Y" rather
 than "ports W, X, Y, Z".

 > +target.torrc ONLINE       => PORT

 Out of curiosity what is the reason for this change?

 > +    control_message = mocking.get_message(UNRECOGNIZED_KEYS_RESPONSE)
 > +    self.assertRaises(stem.socket.InvalidRequest,
 stem.response.convert, "MAPADDRESS", control_message)
 > +    control_message = mocking.get_message(UNRECOGNIZED_KEYS_RESPONSE)
 > +    expected = { "23": "324" }
 > +    control_message = mocking.get_message(PARTIAL_FAILURE_RESPONSE)

 The second control_message assignment is unnecessary. Please inlude a
 blank line above the 'expected' assignment so it's clear that these are
 two tests.

 > +def external_ip(sock):
 > ...
 > +  except:
 > +    pass

 Please explicitely return None. I know that's the default python behavior
 but it's nicer when things are explicit. :)

 Also, the function's pydocs should be very clear that this is an active
 check and that it relies on 'ifconfig.me'.

 > +def negotiate_socks(sock, host, port):
 > +  request = "\x04\x01" + struct.pack("!H", port) + "\x00\x00\x00\x01" +
 "\x00" + host + "\x00"

 This and the following code looks pretty arcane. Mind adding some inline
 comments saying what each step of it does?

 > +  def test_mapaddress(self):

 Shouldn't this require the ONLINE target?

 > +      test.utils.negotiate_socks(s, '1.2.1.2', 80)
 > +      s.sendall(test.utils.ip_request)

 What's at '1.2.1.2'? I'm a little confused what the second line is doing,
 mind commenting it?

 > +      ip_addr = response[response.find("\r\n\r\n"):].strip()

 What does the response commonly look like? (good thing to comment)

 > +      socket.inet_aton(ip_addr) # validate IP

 This might be a good spot for...
 https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/util/connection.py#l33

 Cheers! -Damian

 PS. I know it's not related to this change but when I ran the integ tests
 I got...

 {{{
 test_repurpose_circuit                                       [FAILURE]

 ======================================================================
 ERROR: test_repurpose_circuit
 ----------------------------------------------------------------------
 Traceback:
   File "/home/atagar/Desktop/stem/test/integ/control/controller.py", line
 401, in test_repurpose_circuit
     controller.repurpose_circuit(circ_id, purpose)
   File "/home/atagar/Desktop/stem/stem/control.py", line 1077, in
 repurpose_circuit
     raise stem.socket.InvalidRequest(response.code, response.message)
 InvalidRequest: Unknown purpose "purpose=CONTROLLER"

 ----------------------------------------------------------------------
 }}}

 Thoughts?

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