Opened 5 years ago

Closed 5 years ago

#6951 closed task (implemented)

Implement a wrapper method for MAPADDRESS requests

Reported by: neena Owned by: neena
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (5)

comment:1 Changed 5 years ago by neena

Status: newneeds_review

comment:2 Changed 5 years ago by atagar

Status: needs_reviewneeds_revision

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?

comment:3 in reply to:  2 Changed 5 years ago by neena

Status: needs_revisionneeds_review

Replying 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?)

comment:4 Changed 5 years ago by neena

Also, moved test.utils to test.util for consistency (stem.util exists).

comment:5 Changed 5 years ago by atagar

Resolution: implemented
Status: needs_reviewclosed

Thanks! 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

Note: See TracTickets for help on using tickets.