Implement a wrapper method for MAPADDRESS requests
Implemented the map address wrapper method here.
Things to mention:
- Changed "SocksPort 0" to "SocksPort 29327" from the base torrc. (29327 is random)
- I wrote an external_ip() function in test.utils (a new file) which I'm not using right now. I'll be using this in a bunch of tests soon. It depends on http://ifconfig.me/ip being up.
Related: spec.
Activity
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?
Trac:
Status: needs_review to needs_revisionReplying to 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?) **Trac**: **Status**: needs_revision **to** needs_reviewThanks! Pushed.
ImportError: No module named utils
When moving a module it's a good idea to rm the pyc file, otherwise python will keep happily running the code that no longer exists.
Hmm, isn't this 1111?
Oops! I confused it with the test prompt port which is 2779 (I should probably bring that into this range too...).
AFAIK pass is the standard way to say "If there's any problem while running this code, don't bother doing anything about it".
Right, if you don't want a block to do anything then 'pass' is the right keyword. However, the comment above says...
:returns: externally visible IP address, or None if it isn't able to
... that's why I suggested explicitely returning None.
-
stem.util.connection.is_valid_ip_address(ip_addr)
This returns a boolean so we need this to be in an assertTrue.
That's odd. Is this happening intermittently? or every time? Is it just on my mapaddress-foo branch? Or on master?
It seems to be consistent, and was happening with master too. Happy to discuss this more either here or on a separate ticket. My guess is that the CONTROLLER purpose was added in a later tor version. I'm testing with tor 0.2.1.30 specifically because it's old and hence able to catch this sort of compatability issue. When I run with tor 0.2.3.16 the issue doesn't manifest.
Here's the requested output...
atagar@morrigan:~/Desktop/stem$ telnet localhost 2779 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. AUTHENTICATE 250 OK GETINFO circuit-status 250+circuit-status= 35 BUILT $7EE9EF01F507BB1942CA74F723688AF0E2220F42,PolPotsOptometry,yetiOnIce PURPOSE=GENERAL 34 BUILT $7EE9EF01F507BB1942CA74F723688AF0E2220F42,$6557396CF0EE5B72563A22BCAA0FF26E77FA3D08,t00lhav3n PURPOSE=GENERAL 33 BUILT $16404C671C6503F2C7D9A072DF065EAF9FE7AAF3,$6557396CF0EE5B72563A22BCAA0FF26E77FA3D08,$906C3E72D7D9CD4998CB7596D34CE415B8927B15 PURPOSE=GENERAL 32 BUILT ergebnisoffen,chaoscomputerclub19,assk PURPOSE=GENERAL . 250 OK SETCIRCUITPURPOSE 32 PURPOSE=CONTROLLER 552 Unknown purpose "PURPOSE=CONTROLLER"Cheers! -Damian
Trac:
Resolution: N/A to implemented
Status: needs_review to closed-
- Trac closed
closed