----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/ -----------------------------------------------------------
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/#review364 -----------------------------------------------------------
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/131/#comment192
Note that 0 is False in Python.
How about (untested) > if (uid is None) != (gid is None):
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/131/#comment193
AFAICS _calling_ the demote() function is superfluous and, to me, confusing; we only need to pass the set_ids callable.
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/131/#comment195
setre[ug]id() to make it explicit that both are changed?
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/131/#comment194
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
On Led. 20, 2015, 7:12 odp., Stephen Gallagher wrote:
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 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#file524line294
AFAICS _calling_ the demote() function is superfluous and, to me, confusing; we only need to pass the set_ids callable.
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 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#file524line298
setre[ug]id() to make it explicit that both are changed?
Seems excessive, but ok, sure.
- Stephen
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/#review364 -----------------------------------------------------------
On Jan. 20, 2015, 6:12 p.m., Stephen Gallagher wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/
(Updated Jan. 20, 2015, 6:12 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
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#file524line301
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)
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)
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#file524line294
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.
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 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#file524line298
setre[ug]id() to make it explicit that both are changed?
Stephen Gallagher wrote: Seems excessive, but ok, sure.
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.
- Miloslav
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/#review364 -----------------------------------------------------------
On Led. 21, 2015, 4:49 odp., Stephen Gallagher wrote:
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 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#file524line294
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.)
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.
- Miloslav
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/#review364 -----------------------------------------------------------
On Led. 21, 2015, 4:49 odp., Stephen Gallagher wrote:
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#file524line294
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.
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 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#file524line298
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.
Sorry, I misread the original comment (and frankly completely forgot about setreuid()). I'll fix this in the next version.
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#file524line301
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)
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
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/#review364 -----------------------------------------------------------
On Jan. 21, 2015, 3:49 p.m., Stephen Gallagher wrote:
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 Jan. 21, 2015, 3:49 p.m.)
Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.
Changes -------
Updates based on code review.
Repository: rolekit
Description -------
Allow impersonating a different UID/GID in subprocesses
Diffs (updated) -----
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/#review370 -----------------------------------------------------------
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/131/#comment203
Need to also reset supplementary groups to match user_uid, otherwise root’s will be inherited.
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/131/#comment202
This is now redundant with the check at line 299.
- Miloslav Trmac
On Led. 21, 2015, 4:49 odp., Stephen Gallagher wrote:
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. 21, 2015, 4:39 p.m., Miloslav Trmac wrote:
src/rolekit/async.py, lines 291-292 http://reviewboard-fedoraserver.rhcloud.com/r/131/diff/2/?file=525#file525line291
This is now redundant with the check at line 299.
Hmm, I remember removing that; maybe I missed it when I rebased after dropping the stdin patch. Fixed.
- Stephen
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/#review370 -----------------------------------------------------------
On Jan. 21, 2015, 3:49 p.m., Stephen Gallagher wrote:
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 Jan. 22, 2015, 4:18 p.m.)
Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.
Changes -------
Further changes based on review comments.
Repository: rolekit
Description -------
Allow impersonating a different UID/GID in subprocesses
Diffs (updated) -----
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/#review377 -----------------------------------------------------------
Ship it!
Ship It!
- Thomas Woerner
On Jan. 22, 2015, 4:18 p.m., Stephen Gallagher wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/131/
(Updated Jan. 22, 2015, 4:18 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 Feb. 17, 2015, 5:52 p.m.)
Status ------
This change has been marked as submitted.
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
rolekit-commits@lists.fedorahosted.org