if (uid is None) != (gid is None):

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

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/ ----------------------------------------------------------- (Updated Led. 20, 2015, 7:12 odp.) Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner. Repository: rolekit Description ------- Allow impersonating a different UID/GID in subprocesses Diffs ----- src/rolekit/async.py 0f9ddaac1beb27cebdf41ca0383a62a807c4fcb6 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/ Testing ------- Thanks, Stephen Gallagher

On Led. 20, 2015, 8:48 odp., Miloslav Trmac wrote: > src/rolekit/async.py, line 301 > <http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/1/?file=524#file52... > > 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 Led. 20, 2015, 8:48 odp., Miloslav Trmac wrote: > src/rolekit/async.py, line 294 > <http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/1/?file=524#file52... > > AFAICS _calling_ the demote() function is superfluous and, to me, confusing; we only need to pass the set_ids callable. 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.

>> import os, subprocess >> def set_ids(): os.setregid(1,1); os.setreuid(1,1)

>> p = subprocess.Popen(['/bin/id', '-a'], preexec_fn = set_ids)

>> print os.geteuid()

>> print os.getuid()

>> print os.getegid()

>> print os.getgid()

>>

On Led. 20, 2015, 8:48 odp., Miloslav Trmac wrote: > src/rolekit/async.py, lines 298-299 > <http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/1/?file=524#file52... > > setre[ug]id() to make it explicit that both are changed? Stephen Gallagher wrote: Seems excessive, but ok, sure.

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/ ----------------------------------------------------------- (Updated Led. 21, 2015, 4:49 odp.) Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner. Repository: rolekit Description ------- Allow impersonating a different UID/GID in subprocesses Diffs ----- src/rolekit/async.py 0f9ddaac1beb27cebdf41ca0383a62a807c4fcb6 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/ Testing ------- Thanks, Stephen Gallagher

On Jan. 20, 2015, 7:48 p.m., Miloslav Trmac wrote: > src/rolekit/async.py, line 294 > <http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/1/?file=524#file52... > > AFAICS _calling_ the demote() function is superfluous and, to me, confusing; we only need to pass the set_ids callable. 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. 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.) 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.

On Jan. 20, 2015, 7:48 p.m., Miloslav Trmac wrote: > src/rolekit/async.py, lines 298-299 > <http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/1/?file=524#file52... > > setre[ug]id() to make it explicit that both are changed? Stephen Gallagher wrote: Seems excessive, but ok, sure. 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.

On Jan. 20, 2015, 7:48 p.m., Miloslav Trmac wrote: > src/rolekit/async.py, line 301 > <http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/1/?file=524#file52... > > 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) 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)

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/ ----------------------------------------------------------- (Updated Jan. 21, 2015, 3:49 p.m.) Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner. Repository: rolekit Description ------- Allow impersonating a different UID/GID in subprocesses Diffs ----- src/rolekit/async.py 0f9ddaac1beb27cebdf41ca0383a62a807c4fcb6 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/ Testing ------- Thanks, Stephen Gallagher

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/ ----------------------------------------------------------- (Updated Led. 21, 2015, 4:49 odp.) Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner. Repository: rolekit Description ------- Allow impersonating a different UID/GID in subprocesses Diffs ----- src/rolekit/async.py 0f9ddaac1beb27cebdf41ca0383a62a807c4fcb6 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/ Testing ------- Thanks, Stephen Gallagher