Setting UID (e.g. dropping root privileges)

[tomekwojcik]

Hello,
I'm working on adding _uid special keyword argument and I thought I'd ask for suggestions before actually opening a pull request. The code in question is available here.

Now, as you can see this is clearly a dirty hack at the moment. What I'd like to know is how to properly handle the situation when the parent process isn't owned by root (currently raising RuntimeError). Also, I have to explicitly turn off TTYs on stdin and stdout (which isn't that much of a problem, I guess).

Would you even consider adding this functionality to the library?

Best regards.

EDIT:
An example of the feature in action:

twojcik-mb:sh bilbo$ sudo python -c "import sh; print sh.whoami().stdout.strip(); print sh.whoami(_uid=501).stdout.strip()"
root
bilbo

[amoffat]

Hi Tomek

I like it and would merge this feature. I think the behavior of raising a RuntimeError if non-root tries to use setuid makes sense. The other alternative is to quietly fail (and launch the subprocess as the user that owns the parent process), but this might have security implications if the user was expecting _uid to work.

I'm curious why TTYs need to be turned off though?

Also, be sure to add a unit test in test.py You'll probably want to create a temporary script in your unit test that prints one thing if uid is one value, and another thing if the uid is another value, then test this output. Take a look at how the other tests do this.

[tomekwojcik]

Hi Andrew,
I totally agree on security implications of silencing the error when non-root user is trying to change UID of a process. Personally, I'd much rather see the exception being raised as it indicates a problem somewhere on my end.

As far as turning of the TTYs - if I don't do so the process hangs and I have to ^C after running a command, e.g.

Shire:sh bilbo$ sudo python
Python 2.7.3 (default, Oct 25 2012, 22:00:57)
[GCC 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sh
>>> sh.whoami()
root

>>> sh.whoami(_uid=501)
^CTraceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sh.py", line 731, in __call__
    return RunningCommand(cmd, call_args, stdin, stdout, stderr)
  File "sh.py", line 292, in __init__
    self.wait()
  File "sh.py", line 296, in wait
    self._handle_exit_code(self.process.wait())
  File "sh.py", line 1117, in wait
    pid, exit_code = os.waitpid(self.pid, 0)
KeyboardInterrupt
>>> sh.whoami(_uid=501, _tty_out=False, _tty_in=False)
bilbo

>>> 

I have no idea what's the problem here. Permissions, maybe? Nevertheless, I think that if the docs mention switching off TTYs when using _uid then everything should be fine.

I've been thinking a lot about testing the feature, but I see a small problem here - the test would have to be run by root, so setting UID is possible. Any ideas how to tackle this problem?

One more question has just came to my mind - which Python versions do you wish me to test before opening a pull request? I'm thinking 2.6, 2.7, 3.3.

Anyways, thanks for the lib - it's been very helpful and made my life much easier :).

[amoffat]

Ah I was able to reproduce the TTY/uid issue. However, when I ran my test script, I got an exception:

22:21:02 $> sudo python uid_tty_fail.py 
Traceback (most recent call last):
  File "uid_tty_fail.py", line 3, in <module>
    print sh.whoami(_uid=1000)
  File "/home/amoffat/workspace/pbs/sh.py", line 731, in __call__
    return RunningCommand(cmd, call_args, stdin, stdout, stderr)
  File "/home/amoffat/workspace/pbs/sh.py", line 292, in __init__
    self.wait()
  File "/home/amoffat/workspace/pbs/sh.py", line 296, in wait
    self._handle_exit_code(self.process.wait())
  File "/home/amoffat/workspace/pbs/sh.py", line 310, in _handle_exit_code
    self.process.stderr
sh.ErrorReturnCode_1: 

  RAN: '/usr/bin/whoami'

  STDOUT:


  STDERR:
Traceback (most recent call last):
  File "uid_tty_fail.py", line 3, in <module>
    print sh.whoami(_uid=1000)
  File "/home/amoffat/workspace/pbs/sh.py", line 731, in __call__
    return RunningCommand(cmd, call_args, stdin, stdout, stderr)
  File "/home/amoffat/workspace/pbs/sh.py", line 289, in __init__
    self.call_args, pipe=pipe)
  File "/home/amoffat/workspace/pbs/sh.py", line 855, in __init__
    tmp_fd = os.open(os.ttyname(1), os.O_RDWR)
OSError: [Errno 13] Permission denied: '/dev/pts/3'

If I do what you did in the terminal though, no exception is raised, which is odd. But for this, I think it's clear that the setuid is happening too soon. Can you try moving it closer to the os.execve and see if that works for you?

I've been thinking a lot about testing the feature, but I see a small problem here - the test would have to be run by root, so setting UID is possible. Any ideas how to tackle this problem?

I think it makes sense to wrap that particular unit test in a "skip" decorator. Take a look at requires_posix and how it's used in test.py. The new decorator could be called requires_root and tests if the executing script has uid == 0.

One more question has just came to my mind - which Python versions do you wish me to test before opening a pull request? I'm thinking 2.6, 2.7, 3.3.

All 3 if possible. If you run python sh.py test, it should automate testing all the versions of python you have installed.

Anyways, thanks for the lib - it's been very helpful and made my life much easier :).

Very happy to hear it, I'm glad it's helpful :)

[tomekwojcik]

OK, so I moved the UID setting part before os.execve call and it works :). See the current code here.

I'm gonna perform some tests on the code that is the root cause of my need to change UID (it's a part of a project I'm working on at work) and see if it works as expected.

As far as functional and compatibility tests go - thanks for your input. I'll keep that in mind while writing test code and performing compatibility tests. Targeting Python 3.x isn't a problem for me - I already have the interpreters built and ready to use.

Once I'm done I'll open a pull request so you can review the final code.

[amoffat]

Merged what was in your branch into the 1.2 release and it will go out then. Thanks for your contribution 👍