[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_review
 Priority:  normal  |      Milestone:              
Component:  Stem    |        Version:              
 Keywords:          |         Parent:              
   Points:          |   Actualpoints:              
--------------------+-------------------------------------------------------
Changes (by neena):

  * status:  needs_revision => needs_review


Comment:

 Replying to [comment:2 atagar]:
 > 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)
 > }}}
 Hmm, done.

 > > -    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?
 Every other failure case I've seen till now failed with all the lines
 having an error code. So, this would still apply to those. So, it handles
 everything that I've come across until now fine.

 > 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)
 > }}}
 Did something similar.

 >
 > > - 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".
 Hmm, isn't this 1111?
 https://gitweb.torproject.org/stem.git/blob/HEAD:/test/runner.py#l85
 I'm using 1112 now, shall we change it to a higher number?

 > > +target.torrc ONLINE       => PORT
 >
 > Out of curiosity what is the reason for this change?
 Mmmh, nevermind :/ removed.

 > > +    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.
 Done.

 >
 > > +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. :)

 AFAIK pass is the standard way to say "If there's any problem while
 running this code, don't bother doing anything about it". Unless we need
 to return and get control out of the function immediately, we ought to use
 pass. Because, otherwise every function could end with return.

 I don't mind changing it, but I think it conveys more meaning this way.

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

 > > +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?
 oof, yes.

 > > +      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?

 The test is mapping 1.2.1.2 to ipconfig.me. We're connecting to 1.2.1.2
 and checking that the connection is made to ipconfig.me. (This test would
 be bad if 1.2.1.2 somehow redirected to ipconfig.me or did what it did
 without mapaddress replacing the address... unlikely.)

 Commented.

 >
 > > +      ip_addr = response[response.find("\r\n\r\n"):].strip()
 >
 > What does the response commonly look like? (good thing to comment)
 Added.

 >
 > > +      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
 Ah. Forgot about that. used.

 > 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"

 That's odd. Is this happening intermittently? or every time?
 Is it just on my mapaddress-foo branch? Or on master?

 can you try the following using netcat. Run it...

 {{{
 $ nc localhost 9100
 }}}

 and feed it...

 {{{
 authenticate
 getinfo circuit-status
 setcircuitpurpose 1 purpose=CONTROLLER
 }}}

 Replace 1 in the setcircuitpurpose line with any general circuit id from
 the getinfo request.

 (file a seperate bug?)

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