On January 20th, 2015, 7:48 p.m. UTC, Miloslav Trmac wrote:

src/rolekit/async.py (Diff revision 1)
def input_handler(unused_fd, condition, unused_data):
		294	def demote(user_uid, user_gid):

AFAICS calling the demote() function is superfluous and, to me,  confusing; we only need to pass the set_ids callable.

On January 21st, 2015, 3:21 p.m. UTC, Stephen Gallagher wrote:

I actually looked up how to do this in several places and all the examples I could find insisted that it had to be done this way or else the calling application would also be affected by the permission drop. I'm choosing to trust them.

On January 21st, 2015, 4:35 p.m. UTC, Miloslav Trmac wrote:

Testing disagrees:



import os, subprocess
def set_ids(): os.setregid(1,1); os.setreuid(1,1)
 ... 
p = subprocess.Popen(['/bin/id', '-a'], preexec_fn = set_ids)
 uid=1(bin) gid=1(bin) skupiny=1(bin),0(root) kontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
print os.geteuid()
 0
print os.getuid()
 0
print os.getegid()
 0
print os.getgid()
 0




(and this also shows that supplementary groups need to be dropped.  Will raise both separately in the new patch.)

On January 21st, 2015, 4:44 p.m. UTC, Miloslav Trmac wrote:

On one more thought…
1) The only way those “several places” make sense to me is that calling set_ids() within the parent process, instead of just referring to its name, would indeed affect the parent; that this is a syntax confusion and the existence of demote allows using preexec_fn = some_function_name_and_parentheses() hiding this syntax confusion.  Just a guess, though.
2) Actually, the middle function might be useful to avoid a circular reference between subprocess_future closure <-> the Popen object .  I’m not actually sure how Python does this (would the “process” variable be a part of the closure?) but using a smaller closure is more obviously refcounting-friendly.  So let’s keep it as it is.

1) OK, so preexec_fn needs to be callable with no arguments. So what we're doing here is constructing a callable that has had the correct arguments pre-determined.
2) I actually don't understand any of this sentence at all (I don't have a good understanding of Python low-level), but since you end with "let's keep it as it is", I'm going to just agree :)

On January 20th, 2015, 7:48 p.m. UTC, Miloslav Trmac wrote:

src/rolekit/async.py (Diff revision 1)
def input_handler(unused_fd, condition, unused_data):
		298	os.setgid(user_gid)
		299	os.setuid(user_uid)

setre[ug]id() to make it explicit that both are changed?

On January 21st, 2015, 3:21 p.m. UTC, Stephen Gallagher wrote:

Seems excessive, but ok, sure.

On January 21st, 2015, 4:35 p.m. UTC, Miloslav Trmac wrote:

Specifically setRE[ug]id would be better because set[ug]id also affects the e[ug]id in not-quite-intuitive ways (and in platform-dependent ways, which does not matter for Linux but makes readability worse).  os.setreuid and os.setregid are available.

Sorry, I misread the original comment (and frankly completely forgot about setreuid()). I'll fix this in the next version.

On January 20th, 2015, 7:48 p.m. UTC, Miloslav Trmac wrote:

src/rolekit/async.py (Diff revision 1)
def input_handler(unused_fd, condition, unused_data):
		301	if user_uid and user_gid:

One way to fix: move this check inside set_ids(), and then call Popen(… preexec_fn=set_ids)
Another way, more similar to the current code:

if (user_uid is not None or user_gid is not None): # minimal cleanup related to 0/none and being paranoid
    preexec_fn = set_ids
else:
    preexec_fn = None
…
and then call Popen(…, preexec_fn=preexec_fn)
(change names as you like)

On January 21st, 2015, 4:35 p.m. UTC, Miloslav Trmac wrote:

Oops, let me try to fix the line wraps:
if (user_uid is not None or user_gid is not None): # minimal cleanup related to 0/none and being paranoid
    preexec_fn = set_ids
  else:
    preexec_fn = None
… and then call Popen(…, preexec_fn=preexec_fn) (change names as you like)

I think I covered that in my most recent patch. See my comment above regarding not being able to pass arguments into the preexec_fn.

- Stephen

On January 21st, 2015, 3:49 p.m. UTC, Stephen Gallagher wrote:

Allow impersonating a different UID/GID in subprocesses