Support TAKEOWNERSHIP command

Please add support for the TAKEOWNERSHIP control command in the Controller class.

The attached patch works fine in my manual tests. No automated tests, as I did not find a good way to plug such trivial command in the current test suites.

added component::archived/stem owner::atagar priority::medium resolution::implemented status::closed type::enhancement labels

Trac:
0001-Implement-TAKEOWNERSHIP-as-Controller.take_ownership.patch

Implement TAKEOWNERSHIP as Controller.take_ownership (v1)

Trac:
Status: new to needs_review

See also #7667 (closed).

Hi lunar, thanks for the patch! I like Robert's suggestion about asserting ownership when spawning the tor process - how does this look to you both?

https://gitweb.torproject.org/stem.git/commitdiff/7fba50ff561578f746ceb3383b1bb16d709a14e3

Replying to atagar:

Hi lunar, thanks for the patch! I like Robert's suggestion about asserting ownership when spawning the tor process - how does this look to you both?

https://gitweb.torproject.org/stem.git/commitdiff/7fba50ff561578f746ceb3383b1bb16d709a14e3

In stem/control.py, s/thrugh/through/.

Using sleep 10 as your dummy process in test_take_ownership_via_pid seems a bit risky -- if the computer has a busy or slow (or failing) disk, Tor (or sleep) may take more than 10 seconds to start. Since you're going to kill that process anyway, having it run for a longer time (perhaps five minutes) shouldn't hurt anything.

Your Controller class's support for the TAKEOWNERSHIP command assumes that Stem will always be run in the same process-ID namespace as any Tor process that it is asked to connect to. That's even more risky. When I opened #7667 (closed), I had assumed that Stem already provided a way to start a Tor process and automatically open a control-port connection to it; in that case, it would have been nearly trivial for Stem to pass the ‘owned Tor instance’ flag from the function that launches Tor to the code that connects to its control port.

In stem/control.py, s/thrugh/through/.

fixed

... having it run for a longer time (perhaps five minutes) shouldn't hurt anything.

Ideally I'd like to get rid of all of the sleep() calls in the tests in favor of something like a scheduled executor since they suffer from a similar drawback. However, I'm not familiar with a counterpart for that class in python.

Raised the sleep to a minute. I'd like to keep it reasonably low so the tests can't leave orphaned processes if the kill() request fails.

Your Controller class's support for the TAKEOWNERSHIP command assumes that Stem will always be run in the same process-ID namespace

Good point. Changed the TAKEOWNERSHIP call to only take place if the control socket is for localhost.

https://gitweb.torproject.org/stem.git/commitdiff/120907822f06fd476f2c47b7135e816cd730b1c7

Anything else, or is this good to be resolved?

Replying to atagar:

Your Controller class's support for the TAKEOWNERSHIP command assumes that Stem will always be run in the same process-ID namespace

Good point. Changed the TAKEOWNERSHIP call to only take place if the control socket is for localhost.

https://gitweb.torproject.org/stem.git/commitdiff/120907822f06fd476f2c47b7135e816cd730b1c7

Ah, so it'll only break if someone uses FreeBSD jails or Linux containers (see package lxc) to put Tor in a different PID namespace on the same OS, or accesses a remote Tor instance securely using SSH port forwarding or Tor hidden services with socat or stunnel or ....

I realize that accessing a remote or jailed Tor instance's control port is not likely to be a common use case, and accidentally sending TAKEOWNERSHIP to one should be a bit rarer than that, but your approach is unnecessarily fragile and complex, and it is wrong.

What kind of object does launch_tor return, and how do callers obtain a Controller object from that one?

What kind of object does launch_tor return

It provides a process. launch_tor() is not restricted to making a tor process with a control socket, and even if there is one launch_tor() won't know the password if HashedControlPassword was set.

We could make a more restricted launch_tor* method that provides back a Controller. However, I'd rather invest our effort in making an is_local() check that actually works. You make a good point that it's non-trivial to figure out, but it would be a useful to detect for reasons other than TAKEOWNERSHIP. Suggestions on how to do that are welcome.

and how do callers obtain a Controller object from that one?

There's a few ways. One would be...

tor_process = stem.process.launch_tor_with_config(
  config = {
    'ControlPort': '2778',
    'CookieAuthentication': '1',
  },
)

controller = Controller.from_port(control_port = 2778)
controller.authenticate()

# ... do some stuff...

controller.close()
tor_process.kill()

Replying to atagar:

What kind of object does launch_tor return

It provides a process. launch_tor() is not restricted to making a tor process with a control socket, and even if there is one launch_tor() won't know the password if HashedControlPassword was set.

We could make a more restricted launch_tor* method that provides back a Controller.

This would be useful for any program which has a good reason to use TAKEOWNERSHIP.

However, I'd rather invest our effort in making an is_local() check that actually works. You make a good point that it's non-trivial to figure out, but it would be a useful to detect for reasons other than TAKEOWNERSHIP. Suggestions on how to do that are welcome.

A year or two ago, I would have tried to look for ways to detect that a Tor process is ‘local’ to a controller. Now, I don't even know what ‘local’ should mean -- does it mean that Tor and the controller have the same process-ID namespace? Does it mean that they can send signals to each other? Does it mean that they have the same filesystem namespace? Does it mean that they could potentially share a chunk of memory? Does it mean that lockf or flock or fcntl locks set by one process can be seen by the other?

and how do callers obtain a Controller object from that one?

There's a few ways. One would be...

Ick. This should be wrapped in a launch_captive_tor function, which should return an object which provides a Controller object and a factory object for SOCKS connections (presumably applications will want other things too).

(I sure hope there's a function for that buried somewhere in BridgeT in ooni-probe.)

    runtime_args += ["--__OwningControllerProcess", _get_pid()]

That's absolutely minor, but dropping the '--' would make it easier to spot options that are command-line only and those that are configuration options.

We could make a more restricted launch_tor* method that provides back a Controller.

That's more or less the construct I came up for the Nagios check. See the Tor class (lines 40-117) and its usage (lines 165-174) on https://paste.debian.net/215197/ ; so this would fit my use case.

Trac:
215197

A year or two ago, I would have tried to look for ways to detect that a Tor process is ‘local’ to a controller. Now, I don't even know what ‘local’ should mean

I liked your example about tunneled connections. That's something that it would be nice for is_localhost() to be able to detect. As for split PID namespaces on the same machine, this is the first time I've ever heard of those. How does that even work? Does 'ps' show multiple processes with the pid? Does kill or other pid based commands affect all processes that share that pid?

Shared PID namespaces sound like a terrible idea that would break... well, a lot of things. Guess that's something I should read up on at some point. O brave new world that has such things in it...

Personally I define 'local' as 'I can shell out with commands like ps and kill by its pid to affect it'.

Ick. This should be wrapped in a launch_captive_tor function

I disagree. The user may want to set OwningControllerProcess for a process that won't have a controller. Having these specialized functions for launching then connecting to tor lend a lot of flexibility to stem's users (flexibility that blended functions like TorCtl's connect() lacked, and often making those functions useless).

That's absolutely minor, but dropping the '--' would make it easier to spot options that are command-line only and those that are configuration options.

Oops. That's what I get for having a horribly out of date copy of tor's man page installed on my system. Changed...

https://gitweb.torproject.org/stem.git/commitdiff/4cacd2f13d1cd003db0c15addcf265fa4262ee37

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

config = self.extra_config + [
  ('DataDirectory', self.datadir),
  ('ControlSocket', self.control_socket_path()),
  ('CookieAuthentication', '1'),
  ('CookieAuthFile', self.cookie_auth_file_path()),
  ('SocksPort', 'auto'),
  ('__OwningControllerProcess', str(os.getpid())),
]

args = []
for key, value in config:
args += [key, value]

self.popen = stem.process.launch_tor(
  args=args,
  torrc_path=stem.process.NO_TORRC,
  init_msg_handler=self.handle_init_msg)

Out of curiosity, why are you using launch_tor() rather than launch_tor_with_config() for this? It should both make the code nicer, and avoid having a huge process name.

self.popen = stem.process.launch_tor_with_config(
  config = self.extra_config + {
    'DataDirectory': self.datadir,
    'ControlSocket': self.control_socket_path(),
    'CookieAuthentication': '1',
    'CookieAuthFile': self.cookie_auth_file_path(),
    'SocksPort': 'auto',
  }.
  init_msg_handler=self.handle_init_msg,
  take_ownership = True,
)

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

stem.connection.authenticate_cookie(self.controller, self.cookie_auth_file_path())

You could replace this with...

self.controller.authenticate()

Stem can figure out that the controller's using cookie authentication and where the cookie is located without being told. You could then also drop the stem.connection import.

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

self.pid = int(self.controller.get_info('process/pid'))

You don't need to track this. You have 'self.popen', so this would be equiviant to...

self.popen.pid

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

attempts = 10
while attempts > 0:
  time.sleep(1)
  self.popen.poll()
  attempts -= 1
  try:
    os.kill(self.pid, 0)
  except OSError, err:
    if err.errno == errno.ESRCH:
      break
  if attempts == 5:
    os.kill(self.pid, signal.SIGTERM)
if attempts == 0:
  os.kill(self.pid, signal.SIGKILL)
  self.popen.poll()

Doing this with a poll based approach will make your exit() a bit shower than it needs to be. Are you using python 2.6 or later? If so then this will do the trick...

self.popen.kill()
process.communicate() # block until it's gone

... and if it's python 2.5 then...

os.kill(self.popen.pid, signal.SIGTERM)
process.communicate() # block until it's gone

That's what I do for stem's integ test runner and I've never seen it have a problem. If you're really paranoid then you could throw in an alarm to time out the kill if it blocks (shouldn't be needed, but to find an example look for 'signal' in stem/process.py).

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

c = pycurl.Curl()
c.setopt(pycurl.URL, url)
c.setopt(pycurl.PROXY, socks_host)
...

Neat, I wasn't familiar with pycurl. Do you know if it has been added to the builtin library? It could be handy for future tests.

I definitely need to invest some time in making stem friendlier for these sort of client use cases, preferably with an example (that's what #7505 (closed) is for).

Let me know if you have any ideas that would make stem friendlier to use!

Out of curiosity, why are you using launch_tor() rather than launch_tor_with_config() for this? It should both make the code nicer, and avoid having a huge process name.

In the TAKEOWNERSHIP pattern '__OwningControllerProcess' needs to be given on the command-line. Otherwise reset_conf() would not work. Now that launch_tor_with_config() will do TAKEOWNERSHIP properly, it can be used.

There's another thing though. I think that a Python dictionary is not the proper structure for Tor configuration options. As several options can appear multiple times (Log or Bridge), I think a list of 2-uples is better suited...

Neat, I wasn't familiar with pycurl. Do you know if it has been added to the builtin library? It could be handy for future tests.

It's in the python-pycurl package in Debian. I have not researched other plateforms. Its interface is not that pythonic, but at least it works.

Many thanks for the other advices. :)

There's another thing though. I think that a Python dictionary is not the proper structure for Tor configuration options. As several options can appear multiple times (Log or Bridge), I think a list of 2-uples is better suited...

launch_tor_with_config() accepts a mapping of keys to either a value or list of values for that reason. Please see the function's pydocs.

I'm gonna resolve this since the present TAKEOWNERSHIP should do the trick. I'm happy to accept patches to improve is_localhost() or let us detect split local PID namespaces, but that's separate from this issue.

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

mentioned in issue #7667 (closed)

mentioned in issue #7693 (closed)