Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting the user which we drop permissions to using suid. #370

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aiosmtpd/docs/NEWS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
Added
-----
* Unthreaded Controllers (Closes #160)
* Ability to drop permissions to a user other than ``nobody`` using ``-S``

Fixed/Improved
--------------
Expand Down
22 changes: 16 additions & 6 deletions aiosmtpd/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,18 @@ def _parser() -> ArgumentParser:
default=True,
action="store_false",
help=(
"This program generally tries to setuid ``nobody``, unless this "
"flag is set. The setuid call will fail if this program is not "
"run as root (in which case, use this flag)."
"This program uses setuid to drop permissions unless this flag "
"is set. The setuid call will fail if this program is not run "
"as root (in which case, use this flag)."
),
)
parser.add_argument(
"-S",
"--suid-user",
dest="suid_user",
default="nobody",
help=(
"The user to change to using suid; defaults to ``nobody``."
),
)
parser.add_argument(
Expand Down Expand Up @@ -224,12 +233,13 @@ def main(args: Optional[Sequence[str]] = None) -> None:
file=sys.stderr,
)
sys.exit(1)
nobody = pwd.getpwnam("nobody").pw_uid
suid_user_id = pwd.getpwnam(args.suid_user).pw_uid
try:
os.setuid(nobody)
os.setuid(suid_user_id)
except PermissionError:
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)
Comment on lines 240 to 244
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Expand Down
14 changes: 13 additions & 1 deletion aiosmtpd/tests/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# SPDX-License-Identifier: Apache-2.0

import asyncio
from collections import namedtuple
import logging
import multiprocessing as MP
import os
Expand Down Expand Up @@ -141,6 +142,17 @@ def test_setuid(self, nobody_uid, mocker):
main(args=())
mock.assert_called_with(nobody_uid)

def test_setuid_other(self, nobody_uid, mocker):
other_user = namedtuple(
"pwnam",
["pw_uid", "pw_dir", "pw_shell"],
)(42, "/", "/bin/sh")
mock_getpwnam = mocker.patch("pwd.getpwnam", return_value=other_user)
mock_suid = mocker.patch("os.setuid")
main(args=("-S", "other"))
mock_getpwnam.assert_called_with("other")
mock_suid.assert_called_with(42)

def test_setuid_permission_error(self, nobody_uid, mocker, capsys):
mock = mocker.patch("os.setuid", side_effect=PermissionError)
with pytest.raises(SystemExit) as excinfo:
Expand All @@ -149,7 +161,7 @@ def test_setuid_permission_error(self, nobody_uid, mocker, capsys):
mock.assert_called_with(nobody_uid)
assert (
capsys.readouterr().err
== 'Cannot setuid "nobody"; try running with -n option.\n'
== 'Cannot setuid to "nobody"; try running with -n option.\n'
)

def test_setuid_no_pwd_module(self, nobody_uid, mocker, capsys):
Expand Down