Opened 5 years ago

Closed 5 years ago

#6666 closed task (implemented)

Stem wrapper method for the EXTENDCIRCUIT control command

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

Description

Stem should be able to create new circuits and extend existing ones using the control port.

I've pushed my implementation here

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by neena

  • Status changed from new to needs_review

comment:2 follow-up: Changed 5 years ago by atagar

  • Status changed from needs_review to needs_information

Pushed with some minor tweaks...
https://gitweb.torproject.org/stem.git/commitdiff/74ba2e9

However, as mentioned in the commit description there's a couple things I'm uncertain about...

However, there's still a couple mysteries...

* When I was offline I got the following stacktrace...

======================================================================

ERROR: test_extendcircuit

----------------------------------------------------------------------
Traceback:
  File "/home/atagar/Desktop/stem/test/integ/control/controller.py", line 373, in test_extendcircuit
    circ_id = controller.extend_circuit(0)
  File "/home/atagar/Desktop/stem/stem/control.py", line 1109, in extend_circuit
    raise stem.socket.ProtocolError("EXTENDCIRCUIT returned unexpected response code: %s" % response.code)
ProtocolError: EXTENDCIRCUIT returned unexpected response code: 512
----------------------------------------------------------------------

  However, according to the control-spec the 512 response code is for "Syntax
  error in command argument". That doesn't make sense if we're failing because
  we lack a connection.

* Is the 'path' argument for EXTENDCIRCUIT the circuits that we build through,
  or the relays to be chosen from when building a single hop? The EXTENDCIRCUIT
  description doesn't say, which seems to me to be a weakness in the spec.

comment:3 Changed 5 years ago by atagar

Hi Ravi. Any thoughts on this?

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

Replying to atagar:

However, there's still a couple mysteries...

* When I was offline I got the following stacktrace...

======================================================================

ERROR: test_extendcircuit

----------------------------------------------------------------------
Traceback:
  File "/home/atagar/Desktop/stem/test/integ/control/controller.py", line 373, in test_extendcircuit
    circ_id = controller.extend_circuit(0)
  File "/home/atagar/Desktop/stem/stem/control.py", line 1109, in extend_circuit
    raise stem.socket.ProtocolError("EXTENDCIRCUIT returned unexpected response code: %s" % response.code)
ProtocolError: EXTENDCIRCUIT returned unexpected response code: 512
----------------------------------------------------------------------

  However, according to the control-spec the 512 response code is for "Syntax
  error in command argument". That doesn't make sense if we're failing because
  we lack a connection.

Odd. I get
"551 Couldn't start circuit" when I check manually,
and "ProtocolError: EXTENDCIRCUIT returned unexpected response code: 551" when I run
the tests (after disabling check_online). I'm running "Tor version 0.2.3.24-rc (git-f1f16882e345cd4c)".
If you're still getting this, can you tell me what the value of response.message is.

  • Is the 'path' argument for EXTENDCIRCUIT the circuits that we build through, or the relays to be chosen from when building a single hop? The EXTENDCIRCUIT description doesn't say, which seems to me to be a weakness in the spec.

}}}

It's the list of relays with which the circuit should be extended.
Extending "3 BUILT a,b,c" with "EXTENDCIRCUIT 3 x,y,z" will make it
"3 BUILT a,b,c,x,y,z"

comment:5 Changed 5 years ago by neena

  • Owner changed from neena to atagar
  • Status changed from needs_information to assigned

comment:6 Changed 5 years ago by atagar

  • Owner changed from atagar to neena

This looks to now be moot due to the introduction of an ONLINE requirement...

atagar@morrigan:~/Desktop/stem$ ./run_tests.py --integ --test test.integ.control.controller
...
test_extendcircuit (requires online target)                  [SKIPPED]

Usually I run tests with 0.2.1.30 to make sure we aren't introducing problems with older tor versions, but when I call EXTENDCIRCUIT with the equivalent call from that stacktrace I'm not getting a 512 response. Rather, it's a 552...

atagar@morrigan:~/Desktop/stem$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.

AUTHENTICATE
250 OK

GETINFO version
250-version=0.2.1.30
250 OK

EXTENDCIRCUIT 0 purpose=general
552 No such router "purpose=general"

And with tor 0.2.3.16 it's a 551...

atagar@morrigan:~/Desktop/stem$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.

AUTHENTICATE
250 OK

GETINFO version
250-version=0.2.3.16-alpha-dev (git-8be6058d8f31e578)
250 OK

EXTENDCIRCUIT 0 purpose=general
551 Couldn't start circuit

Maybe we should check when the 'purpose=' argument was introduced, and have the Controller do a version check on that before trying to use it?

comment:7 Changed 5 years ago by atagar

  • Keywords controller added

comment:8 Changed 5 years ago by robinson

My testing shows that this code works, as is, with limitations. First, purpose= was added before tor 0.1.1.15-rc, so it is not likely to be a problem.

However, the EXTENDCIRCUIT syntax changed between 0.2.1.x and 0.2.2.y (y > 9), specifically, commit ac68704f07c2b703. Before 0.2.2.8-alpha, ServerSpec was a required parameter to EXTENDCIRCUIT. After 0.2.2.9-alpha, ServerSpec is optional. So, response 512 (missing parameter) or 552 (unknown router) in 0.2.1.x, depending whether purpose= is included.

The "551 Couldn't start circuit" in 0.2.2.y and later seems to be a transitory error. Sometimes the daemon fails to open a new circuit, it happens. Try running the Controller ONLINE tests multiple times and some methods fail some times.

So, at worst, do we need a version check in extend_circuit to give more explicit exceptions for an obsolete version of tor?

comment:9 Changed 5 years ago by atagar

  • Status changed from assigned to needs_review

My testing shows that this code works, as is, with limitations. First, purpose= was added before tor 0.1.1.15-rc, so it is not likely to be a problem.

However, the EXTENDCIRCUIT syntax changed between 0.2.1.x and 0.2.2.y (y > 9)...

Ah ha, thanks. :)

Pushed a change to address this. Thoughts?

https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/b48cfa9a8f3b00d4783465f5db5b2b8824800370

comment:10 Changed 5 years ago by robinson

1) I would use "path" over "ServerSpec" in the exception message(s), since users of stem will be concerned about the names of parameters to extend_circuit, not the params to EXTENDCIRCUIT.

2) Is the "when the circuit id is non-zero" exception needed? The daemon should already return "512 No router names provided". Would it be better to include a 512 status handler in the existing 552 status handler after the self.msg() call? I mean 'if response.code in ("512", "552")' with the same generic InvalidRequest init.

The reason for preemptively checking the version compatibility is that stem is acting as a meta-filter for multiple tor versions. But, the path requirement for non-zero CircuitID goes back to before tor-0.1.0.1-rc (commit 35953edae073e69e). I vote to stay out of the way of the daemon error responses unless we can make them more meaningful or remove uncertainty about the origin of the error (e.g. different versions have different requirements).

comment:11 Changed 5 years ago by atagar

  • Resolution set to implemented
  • Status changed from needs_review to closed

Thanks! Pushed...

https://gitweb.torproject.org/stem.git/commitdiff/6f2c1c33931b6fda738468d03ee0f835f0caca6b

1) I would use "path" over "ServerSpec" in the exception message(s), since users of stem will be concerned about the names of parameters to extend_circuit, not the params to EXTENDCIRCUIT.

Good point. Changed.

2) Is the "when the circuit id is non-zero" exception needed? The daemon should already return "512 No router names provided"...

Ok, you've convinced me. Doing it let us provide a better message and avoid a request to tor we know will fail, but those are pretty weak reasons.

Note: See TracTickets for help on using tickets.