Opened 6 years ago

Closed 5 years ago

#4896 closed enhancement (fixed)

Chroot set up for stem

Reported by: gsathya Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: gsathya.ceg@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I'm going to bounce of a couple of ideas for emulating a chroot set up. I'll attach the relevant branches to the ideas. Also, most of the code is just temp stuff to check which idea looks the least ugly.

Ok, so starting off -

Idea 1 (This is the one atagar mentioned) - https://gitweb.torproject.org/user/gsathya/stem.git/shortlog/refs/heads/chroot

In stem/socket.py,

Define a stripping_function(original_recv, prefix, control_file)
Arguments -

-- original_recv is the original function that we want to create a partial function of; in our case it is recv_message(control_file)
-- prefix is the string that should be stripped out of the ControlMessage returned by recv_message(control_file)
-- control_file is the argument passed to the original function, recv_message(control_file)

This function returns a ControlMessage which has prefix stripped from it.

In test/runner.py,

Add a constant CHROOT_ENV that is set by run_tests.py when the --chroot arg is used.


In Runner.start(),

Check if CHROOT_ENV is set. If it is then do,
stem.socket.recv_message = functools.partial(stem.socket.stripping_function, self._original_recv, self.get_test_dir())


In Runner.stop(),

Check if CHROOT_ENV is set. If it is then do,
stem.socket.recv_message = self._original_recv

Idea 2 -
https://gitweb.torproject.org/user/gsathya/stem.git/shortlog/refs/heads/chroot2

In stem/socket.py,

Define a function strip(self, prefix) in ControlMessage.
This function is similar to the stripping_function explained above. It takes a prefix as an argument and strips it from the content.


recv_message(control_file) is modified -

-- Add an extra argument "prefix" which is False by default. We set this to a string which we want to strip from the content.
-- return ControlMessage(parsed_content, raw_content, prefix) rather than ControlMessage(parsed_content, raw_content)

In test/runner.py.

Initialise,

self._chroot = False
self._original_recv = stem.socket.recv_message


In Runner.start(),

Check if CHROOT_ENV is set. If it is then do,
stem.socket.recv_message = functools.partial(stem.socket.recv_message, prefix = self.get_test_dir())


In Runner.stop(),

Check if CHROOT_ENV is set. If it is then do,
stem.socket.recv_message = self._original_recv

Note -

  • This idea follows from Idea 1.
  • This idea cleans up a bit on the monkey patching by defining a proper method strip() in ControlMessage.
  • But this changes the recv_message() by adding an extra argument "prefix" which is kinda ugly.

Idea 3 -
https://gitweb.torproject.org/user/gsathya/stem.git/shortlog/refs/heads/chroot1

In stem/socket.py,

Define a constant CHROOT_PREFIX = None. This constant is set to the prefix that needs to stripped from the content. This is done by stem.test.Runner
If the CHROOT_PREFIX is not None, then the ControlMessage.strip() function is called.

In test/runner.py,

In Runner.start()

Check if CHROOT_ENV is set. If it is then do,

stem.socket.CHROOT_PREFIX = self.get_test_dir()

In Runner.stop()

Check if CHROOT_ENV is set. If it is then do,

stem.socket.CHROOT_PREFIX = None

Note -

  • This idea follows from Idea 2.
  • This involves no monkey patching. This is more like flicking on a switch.
  • I like this the best, unless I overlooked something really obvious which renders this useless.

I haven't defined the strip()/stripping_function() yet, i'm not entirely sure what to do in it. It's just a bunch of comments of my vague understanding of it.

If I didn't really make much sense, I blame it on my insomnia trying to get this to work. And also it's almost midnight here.

Child Tickets

Change History (8)

comment:1 Changed 6 years ago by atagar

As mentioned on irc I'm juggling a few things so I haven't been able to look too closely at the code, but I can discuss a few things I spotted here...

In stem/socket.py, Define a stripping_function(original_recv, prefix, control_file)...

In the current master branch there's a "test/mocking.py" which I'll be using to make monkey patching more readable and less error prone (via a 'revert all monkey patching' function).

The stripping_function isn't of use to stem users so it should be in test/mocking.py instead.

original_recv is the original function...

On first thought I'm not sure why you'd need this arg since there's only a single recv that we might be wanting to overwrite, though maybe this'll make sense when I look closer at the branch...

Add a constant CHROOT_ENV that is set by run_tests.py when the --chroot arg is used.

I'm not sure why this needs to be a global rather than just an arg to start(). Also, as mentioned on irc this should probably be done via something like "--target ATTR_CHROOT" rather than adding a new run_tests.py arg.

I have another attribute that I'm about to add, so I'd imagine that we'll have an ATTR_ALL later. With this the user could say "--target=CONN_ALL,ATTR_ALL" to run all connection types with every combination of run attributes. A damn lengthy integ test but it would be a good workout for tor. :)

Define a function strip(self, prefix) in ControlMessage.

From a coding point of view that does sound much nicer. However, it's a bad idea to introduce testing code into the library. There was one spot where I broke that rule (an override function for "stem.util.system.call()") and I'm currently trying to remove it...

If I didn't really make much sense, I blame it on my insomnia trying to get this to work. And also it's almost midnight here.

Ack! I didn't mean for this to disrupt your sleep. If you'd like I can write an alternate patch that uses 'mocking.py' and you can see if you like it better or not.

I'll try to look into the code in the next few days unless something in the above helps you zero in on an ideal solution.

Cheers! -Damian

comment:2 in reply to:  1 Changed 6 years ago by gsathya

Replying to atagar:

In stem/socket.py, Define a stripping_function(original_recv, prefix, control_file)...

In the current master branch there's a "test/mocking.py" which I'll be using to make monkey patching more readable and less error prone (via a 'revert all monkey patching' function).

The stripping_function isn't of use to stem users so it should be in test/mocking.py instead.

Sure. I'll change that. But if we use Idea #3, we don't have to muck around with monkey patching.

original_recv is the original function...

On first thought I'm not sure why you'd need this arg since there's only a single recv that we might be wanting to overwrite, though maybe this'll make sense when I look closer at the branch...

We need original_recv because I'm overwriting stem.socket.recv_message in Runner.start() to a partial function-- stem.socket.recv_message = functools.partial(stem.socket.stripping_function, self._original_recv, self.get_test_dir())

I could do stem.socket.recv_message = functools.partial(stem.socket.stripping_function, stem.socket.recv_message, self.get_test_dir())
Is this what you meant? Or did you want me to remove original_recv completely? The latter isn't possible because while closing down the Tor instance, in Runner.stop(), I need to revert stem.socket.recv_message to the original unmodified function which we have stored in original_recv -- stem.socket.recv_message = self._original_recv

Add a constant CHROOT_ENV that is set by run_tests.py when the --chroot arg is used.

I'm not sure why this needs to be a global rather than just an arg to start(). Also, as mentioned on irc this should probably be done via something like "--target ATTR_CHROOT" rather than adding a new run_tests.py arg.

Like Runner.start(self, connection_type = DEFAULT_TOR_CONNECTION, quiet = False, prefix = False)?

And then in run_tests.py, I could do

integ_runner.start(connection_type = connection_type, prefix = True)

I didn't want to do this because I'd be making a change to Runner.start which is used everywhere, and "prefix" has no meaning here. But, I like this better. I'll make the changes. Thanks!

Define a function strip(self, prefix) in ControlMessage.

From a coding point of view that does sound much nicer. However, it's a bad idea to introduce testing code into the library. There was one spot where I broke that rule (an override function for "stem.util.system.call()") and I'm currently trying to remove it...

Yeah got it. I don't like this much either.

If I didn't really make much sense, I blame it on my insomnia trying to get this to work. And also it's almost midnight here.

Ack! I didn't mean for this to disrupt your sleep. If you'd like I can write an alternate patch that uses 'mocking.py' and you can see if you like it better or not.

Sure. But I want to try it first. If I mess it up or take up too much time, then you can go ahead and finish it up :)

I'll try to look into the code in the next few days unless something in the above helps you zero in on an ideal solution.

Thanks! :)

comment:3 Changed 6 years ago by gsathya

Cc: gsathya.ceg@… added

Define a function strip(self, prefix) in ControlMessage.

From a coding point of view that does sound much nicer. However, it's a bad idea to introduce testing code into the library. >>There was one spot where I broke that rule (an override function for "stem.util.system.call()") and I'm currently trying to >>remove >it...

Yeah got it. I don't like this much either.

Ok that was wrong. I need this for Idea-3.

So it's either monkey patching(Idea 1) vs introducing a strip() function to stem.socket.ControlMessage?(Idea 3).

I think adding a strip() function is the lesser of two evils. I vote for Idea 3.

comment:4 Changed 6 years ago by atagar

I think adding a strip() function is the lesser of two evils. I vote for Idea 3.

Ok. A stem.socket strip() function isn't useful to library users so I'll give a test/mocking.py alternative a shot, I should be able to get to it some time next week (unless you still wanted to take a stab at it).

Cheers! -Damian

comment:5 Changed 6 years ago by atagar

Status: newneeds_review

Hi Sathyanarayanan, sorry about the delay. Here's an alternative implementation - thoughts?
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/2a7c872ddad9d84c38f183581f75d64f6fa5c684

comment:6 Changed 6 years ago by gsathya

Oops looks like I didn't post here. This looks good. Has it been merged? Also, would it be possible to write a chroot decorator? I got this idea after look at OONI, where there's a @torify decorator(which is actually broken)

Something that would work like -
@chroot
def foo()

and it would chroot that function alone.

comment:7 Changed 6 years ago by atagar

This looks good. Has it been merged?

Nope, I was waiting for you to look it over first. Also, there's still more work to do here. The chroot integ target doesn't pass since the library doesn't yet account for chroot setups. A few options that come to mind are...

  • Simply have a chroot global which users set and the library consults to expand paths. This is the naive solution that I used for arm and for most users this is all that we need. It's trivial to implement but won't account for users who have multiple tor instances, each with their own chroot.
  • Make the chroot path an attribute of the ControlSocket. I have mixed feelings about this since it would address the above issue, but it isn't really a socket property and is meaningless if we're connecting to a remote host. If we did add this there then I'd propose three functions...

is_localhost() - true if the attached instance is on our system, false if it's remote
get_chroot_prefix()
set_chroot_prefix()

Thoughts?

Also, would it be possible to write a chroot decorator?

I'm not familiar with python's decorators - I'll read up on them in a bit. Chroot is an attribute of the test run, rather than a particular test so I'm not sure of their benefit here. We want to confirm that all of the integ tests pass when we have a chroot setup, not just specific ones.

Cheers! -Damian

comment:8 Changed 5 years ago by atagar

Resolution: fixed
Status: needs_reviewclosed

Pushed the CHROOT integ target along with several changes to stem and its tests. Most handling for chroots will be in the Controller, for the current plans around this see...
https://gitweb.torproject.org/stem.git/commit/636fd3704d6ddcba6a2a780119570fc2e25dd77f

Note: See TracTickets for help on using tickets.