Allow setting the user which we drop permissions to using suid. #370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 1706 1707 +1
Branches 310 310
=========================================
+ Hits 1706 1707 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Just a small change in regards to the documentation. And another optional change.
Otherwise, I'm +1 on this. I went ahead and checked it out locally and verified that the tests fail when the code is as-is on the main branch.
I do have a slight concern that there might be a better way to have the other connections made and then drop privileges to nobody... but I don't think that's necessarily a blocker here.
(FYI I used to run test clinics for the Salt project - I'm running my own, and I went through this one at https://twitch.tv/wayneswonderarium tonight if anyone wants to catch the rerun. Also noticed a failure on mac but I don't think that was related to this PR -- was probably just a flaky issue with mac)
| print( | ||
| 'Cannot setuid "nobody"; try running with -n option.', file=sys.stderr | ||
| f'Cannot setuid to "{args.suid_user}"; try running with -n option.', | ||
| file=sys.stderr, | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
suggestion (non-blocking) Since we're going ahead and exiting with status 1 we could go ahead and simply sys.exit(f"Cannot ...") -- and use the (documented) functionality of sys.exit. However one of our tests does check that the exception .value.code == 1, and it would have to equal the error message instead.
Not a big deal, but that would simplify things here just a bit.
There was a problem hiding this comment.
This feels very tangential to this PR. Personally, I feel like the explicit print and exit code is more readable -- I've never seen the sys.exit("...") formulation, though it's by no means new!
So I'm not going to include this change in the PR, because I'm not entirely sold on it.
|
Thanks for the review! I've repushed with your documentation change.
I don't think there is -- neither |
|
Gentle nudge on this -- anything I can do to help move this forward? |
What do these changes do?
We may want a different user than
nobodywhen dropping permissions -- for example, if we intend to connect to a PostgreSQL database which doesidentauthentication.Are there changes in behavior for the user?
No behaviour change.
Checklist
{py36,py37,py38,py39}-{nocov,cov,diffcov}, qa, docs{py36,py37,py38,py39}-{nocov,cov,diffcov}{py36,py37,py38,py39}-{nocov,cov,diffcov}, pypy3-{nocov,cov}, qa, docs{py36,pypy3}-{nocov,cov,diffcov}, qapy36-{nocov,cov,diffcov}, qa, docsNEWS.rstfile