bpo-36046: Add user and group parameters to subprocess #11950

patrick-mclean · 2019-02-20T01:26:35Z

This adds user and groups parameters to the subprocess.Popen constructor, which allows dropping privileges when running a subprocess.

https://bugs.python.org/issue36046

the-knights-who-say-ni · 2019-02-20T01:26:37Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Doc/library/subprocess.rst

Lib/subprocess.py

Modules/_posixsubprocess.c

gpshead · 2019-02-20T02:12:18Z

fyi - we will need the CLA to be signed to accept this PR (see the early CLA bot comment)

patrick-mclean · 2019-02-20T02:47:21Z

fyi - we will need the CLA to be signed to accept this PR (see the early CLA bot comment)

I signed the CLA right before submitting this PR, it just hasn't picked up on that yet. Hopefully it will pick it up soon.

patrick-mclean · 2019-02-20T03:34:46Z

Updated with all recommended changes, and fixed the documentation issue that was causing Travis and Azure to fail.

Doc/library/subprocess.rst

Lib/subprocess.py

Lib/test/test_subprocess.py

Modules/_posixsubprocess.c

Lib/test/test_subprocess.py

patrick-mclean · 2019-02-21T00:43:34Z

Just pushed more changes based on feedback. This push does add PyMem_RawMalloc() and PyMem_RawFree() calls in subprocess_fork_exec to avoid allocating 256KB on the stack as it was doing before.

Doc/library/subprocess.rst

Lib/subprocess.py

Lib/test/test_subprocess.py

Lib/subprocess.py

patrick-mclean · 2019-02-22T02:42:33Z

Pushed another update based on the latest round of feedback.

patrick-mclean · 2019-02-28T02:17:38Z

Updated again, rebased against the current master.

I addressed all the feedback so far. It now uses the feature check macros, and will raise ValueError if the features are missing. The groups parameter has been split to group and extra_groups, setgroups() will not be called if extra_groups is None.

I also added negative tests to make sure that ValueError is raised on systems where these features are missing.

I ran the tests locally with superuser privileges to make sure they actually work when setgroups() actually succeeds rather than returning EPERM.

As a side note, if we want to test the setgroups() in CI, It could potentially be possible to test privileged operations in CI by running the tests in a user namespace.

izbyshev

Thank you for your work, Patrick. Looks good to me except for several small issues (mostly cosmetic) and the issue with uid_t/gid_t types.

izbyshev · 2019-03-02T18:07:43Z

Modules/_posixsubprocess.c

I think you don't need to go into trouble of raising an exception here. The availability of setgroups is checked on Python side, and AFAIK there is no public API to call into the C helper directly (@gpshead: is this right?), so you can remove #else clause and move the if statement under #ifdef (i.e. make it the same as when you call setgroups).

This applies to setregid and setreuid as well.

It is still useful to raise in this situation as it helps debug internal API use problems.

Maybe raise something more serious than a ValueError then? It's effectively an assertion failure if we got here.

From the description in the docs, it looks like SystemError is probably appropriate here, since it's basically a bug in the library if it reaches this (or someone is doing something they shouldn't).

I could also use RuntimeError here as well, if SystemError is not appropriate.

IMHO SystemError is appropriate. You may raise it via PyErr_BadInternalCall.

Lib/test/test_subprocess.py

Lib/subprocess.py

Modules/_posixsubprocess.c

patrick-mclean · 2019-03-04T21:44:21Z

Updated again, rebased against current master.

Made the minor changes suggested above
Moved to using _Py_Uid_Converter/_Py_Gid_Converter as discussed above.
Made the C code use PyErr_BadInternalCall() to raise SystemError if an attempt to use setreuid()/setregid()/setgroups() on a platform that does not support it makes it to C code.
Moved the negative tests to skipUnless

Lib/test/test_subprocess.py

Lib/subprocess.py

….Popen This adds a `user` parameter to the Popen constructor that will call setreuid() in the child before calling exec(). This allows processes running as root to safely drop privileges before running the subprocess without having to use a preexec_fn. This also adds a `group` parameter that will call setregid() in the child process before calling exec(). Finally an `extra_groups` parameter was added that will call setgroups() to set the supplimental groups.

patrick-mclean requested a review from gpshead as a code owner February 20, 2019 01:26

the-knights-who-say-ni added the CLA not signed label Feb 20, 2019

bedevere-bot added the awaiting review label Feb 20, 2019

patrick-mclean force-pushed the subprocess-user-group-parameters branch 2 times, most recently from 788067d to 06d99d5 Compare February 20, 2019 01:54

gpshead reviewed Feb 20, 2019

View reviewed changes

Lib/subprocess.py Outdated Show resolved Hide resolved

Lib/subprocess.py Outdated Show resolved Hide resolved

Lib/subprocess.py Outdated Show resolved Hide resolved

Modules/_posixsubprocess.c Outdated Show resolved Hide resolved

Modules/_posixsubprocess.c Outdated Show resolved Hide resolved

gpshead self-assigned this Feb 20, 2019

patrick-mclean force-pushed the subprocess-user-group-parameters branch 3 times, most recently from 1781568 to 4121a81 Compare February 20, 2019 03:23

patrick-mclean force-pushed the subprocess-user-group-parameters branch 4 times, most recently from 463ed0d to 8e9ac28 Compare February 20, 2019 05:01

izbyshev reviewed Feb 20, 2019

View reviewed changes

Lib/test/test_subprocess.py Outdated Show resolved Hide resolved

patrick-mclean force-pushed the subprocess-user-group-parameters branch from 8e9ac28 to ce2ac40 Compare February 21, 2019 00:37

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 21, 2019

patrick-mclean force-pushed the subprocess-user-group-parameters branch 5 times, most recently from e70bf5b to 2b90745 Compare February 21, 2019 02:32

gpshead reviewed Feb 22, 2019

View reviewed changes

Doc/library/subprocess.rst Outdated Show resolved Hide resolved

Lib/subprocess.py Outdated Show resolved Hide resolved

Lib/test/test_subprocess.py Outdated Show resolved Hide resolved

Lib/subprocess.py Outdated Show resolved Hide resolved

Lib/subprocess.py Outdated Show resolved Hide resolved

patrick-mclean force-pushed the subprocess-user-group-parameters branch 2 times, most recently from e094812 to d0dad2d Compare February 22, 2019 02:15

patrick-mclean force-pushed the subprocess-user-group-parameters branch 7 times, most recently from f9f8fce to de8a836 Compare February 28, 2019 01:58

patrick-mclean force-pushed the subprocess-user-group-parameters branch from de8a836 to 55393eb Compare February 28, 2019 02:24

izbyshev reviewed Mar 2, 2019

View reviewed changes

patrick-mclean force-pushed the subprocess-user-group-parameters branch 2 times, most recently from 9751206 to 2e68e49 Compare March 4, 2019 21:20

vadmium reviewed Mar 15, 2019

View reviewed changes

Lib/test/test_subprocess.py Outdated Show resolved Hide resolved

Lib/subprocess.py Outdated Show resolved Hide resolved

patrick-mclean force-pushed the subprocess-user-group-parameters branch from 2e68e49 to 2528076 Compare March 16, 2019 00:48

gpshead added 3 commits September 12, 2019 17:33

Don't pass via wrong number of args TypeError.

6359b42

Use versionadded 3.9, the feature window for 3.8 is closed.

812fc45

resolve merge conflict

ae0cc15

gpshead added sprint type-feature A feature request or enhancement labels Sep 12, 2019

gpshead approved these changes Sep 12, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Sep 12, 2019

gpshead merged commit 2b2ead7 into python:master Sep 12, 2019

bedevere-bot removed the awaiting merge label Sep 12, 2019

Uh oh!

bpo-36046: Add user and group parameters to subprocess #11950

bpo-36046: Add user and group parameters to subprocess #11950

Uh oh!

Conversation

patrick-mclean commented Feb 20, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gpshead commented Feb 20, 2019

Uh oh!

patrick-mclean commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick-mclean commented Feb 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrick-mclean commented Feb 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrick-mclean commented Feb 22, 2019

Uh oh!

patrick-mclean commented Feb 28, 2019

Uh oh!

izbyshev left a comment

Choose a reason for hiding this comment

Uh oh!

izbyshev Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

gpshead Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

izbyshev Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

patrick-mclean Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

izbyshev Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrick-mclean commented Mar 4, 2019

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

patrick-mclean commented Feb 20, 2019 •

edited by bedevere-bot

Loading

patrick-mclean commented Feb 20, 2019 •

edited

Loading