diff --git a/actions/actions b/actions/actions index c1ce7b1dd..9264b5cfe 100755 --- a/actions/actions +++ b/actions/actions @@ -8,6 +8,7 @@ import json import logging import os import sys +import traceback import typing import plinth.log @@ -26,16 +27,25 @@ def main(): parser = argparse.ArgumentParser() parser.add_argument('module', help='Module to trigger action in') parser.add_argument('action', help='Action to trigger in module') + parser.add_argument('--write-fd', type=int, default=1, + help='File descriptor to write output to') + parser.add_argument('--no-args', default=False, action='store_true', + help='Do not read arguments from stdin') args = parser.parse_args() try: try: - arguments = json.loads(sys.stdin.read()) + arguments = {'args': [], 'kwargs': {}} + if not args.no_args: + input_ = sys.stdin.read() + if input_: + arguments = json.loads(input_) except json.JSONDecodeError as exception: raise SyntaxError('Arguments on stdin not JSON.') from exception return_value = _call(args.module, args.action, arguments) - print(json.dumps(return_value)) + with os.fdopen(args.write_fd, 'w') as write_file_handle: + write_file_handle.write(json.dumps(return_value)) except PermissionError as exception: logger.error(exception.args[0]) sys.exit(EXIT_PERM) @@ -91,7 +101,8 @@ def _call(module_name, action_name, arguments): 'exception': { 'module': type(exception).__module__, 'name': type(exception).__name__, - 'args': exception.args + 'args': exception.args, + 'traceback': traceback.format_tb(exception.__traceback__) } } diff --git a/container b/container index f791f8f97..7884a2913 100755 --- a/container +++ b/container @@ -650,8 +650,10 @@ def _setup_users(image_file): str(gid), 'plinth'], stdout=subprocess.DEVNULL) logger.info('In container: Setting up sudo for users "fbx" and "plinth"') - sudo_config = 'plinth ALL=(ALL:ALL) NOPASSWD:SETENV : ' \ - '/usr/share/plinth/actions/* , /freedombox/actions/*\n' \ + sudo_config = 'Cmnd_Alias FREEDOMBOX_ACTION_DEV = /usr/share/plinth/' \ + 'actions/actions, /freedombox/actions/actions\n' \ + 'Defaults!FREEDOMBOX_ACTION_DEV closefrom_override\n' \ + 'plinth ALL=(ALL:ALL) NOPASSWD:SETENV : FREEDOMBOX_ACTION_DEV\n' \ 'fbx ALL=(ALL:ALL) NOPASSWD : ALL\n' _runc(image_file, ['tee', '/etc/sudoers.d/01-freedombox-development'], input=sudo_config.encode(), stdout=subprocess.DEVNULL) diff --git a/data/etc/sudoers.d/plinth b/data/etc/sudoers.d/plinth index 473da750a..37fbbef1b 100644 --- a/data/etc/sudoers.d/plinth +++ b/data/etc/sudoers.d/plinth @@ -2,7 +2,9 @@ # Allow plinth user to run plinth action scripts with superuser privileges # without needing a password. # -plinth ALL=(ALL:ALL) NOPASSWD:/usr/share/plinth/actions/* +Cmnd_Alias FREEDOMBOX_ACTION = /usr/share/plinth/actions/actions +Defaults!FREEDOMBOX_ACTION closefrom_override +plinth ALL=(ALL:ALL) NOPASSWD:FREEDOMBOX_ACTION # # On FreedomBox, allow all users in the 'admin' LDAP group to execute diff --git a/plinth/actions.py b/plinth/actions.py index 68c17c241..24837a2c5 100644 --- a/plinth/actions.py +++ b/plinth/actions.py @@ -84,6 +84,7 @@ import os import re import shlex import subprocess +import threading from plinth import cfg from plinth.errors import ActionError @@ -281,20 +282,113 @@ def privileged(func): def wrapper(*args, **kwargs): module_name = _get_privileged_action_module_name(func) action_name = func.__name__ - json_args = json.dumps({'args': args, 'kwargs': kwargs}) - return_value = superuser_run('actions', [module_name, action_name], - input=json_args.encode()) - return_value = json.loads(return_value) - if return_value['result'] == 'success': - return return_value['return'] - - module = importlib.import_module(return_value['exception']['module']) - exception = getattr(module, return_value['exception']['name']) - raise exception(*return_value['exception']['args']) + return _run_privileged_method_as_process(module_name, action_name, + args, kwargs) return wrapper +def _run_privileged_method_as_process(module_name, action_name, args, kwargs): + """Execute the privileged method in a sub-process with sudo.""" + run_as_user = kwargs.pop('_run_as_user', None) + run_in_background = kwargs.pop('_run_in_background', False) + + read_fd, write_fd = os.pipe() + os.set_inheritable(write_fd, True) + + # Prepare the command + command = ['sudo', '--non-interactive', '--close-from', str(write_fd + 1)] + if run_as_user: + command += ['--user', run_as_user] + + if cfg.develop: + command += [f'PYTHONPATH={cfg.file_root}'] + + command += [ + os.path.join(cfg.actions_dir, 'actions'), module_name, action_name, + '--write-fd', + str(write_fd) + ] + + proc_kwargs = { + 'stdin': subprocess.PIPE, + 'stdout': subprocess.PIPE, + 'stderr': subprocess.PIPE, + 'shell': False, + 'pass_fds': [write_fd], + } + if cfg.develop: + # In development mode pass on local pythonpath to access Plinth + proc_kwargs['env'] = {'PYTHONPATH': cfg.file_root} + + _log_action(module_name, action_name, run_as_user, run_in_background) + + proc = subprocess.Popen(command, **proc_kwargs) + os.close(write_fd) + + buffers = [] + # XXX: Use async to avoid creating a thread. + read_thread = threading.Thread(target=_thread_reader, + args=(read_fd, buffers)) + read_thread.start() + + wait_args = (module_name, action_name, args, kwargs, proc, command, + read_fd, read_thread, buffers) + if not run_in_background: + return _wait_for_return(*wait_args) + + wait_thread = threading.Thread(target=_wait_for_return, args=wait_args) + wait_thread.start() + + +def _wait_for_return(module_name, action_name, args, kwargs, proc, command, + read_fd, read_thread, buffers): + """Communicate with the subprocess and wait for its return.""" + log_error = kwargs.pop('_log_error', True) + json_args = json.dumps({'args': args, 'kwargs': kwargs}) + + output, error = proc.communicate(input=json_args.encode()) + read_thread.join() + if proc.returncode != 0: + logger.error('Error executing command - %s, %s, %s', command, output, + error) + raise subprocess.CalledProcessError(proc.returncode, command) + + try: + return_value = json.loads(b''.join(buffers)) + except json.JSONDecodeError: + logger.error( + 'Error decoding action return value %s..%s(*%s, **%s): %s', + module_name, action_name, args, kwargs, return_value) + raise + + if return_value['result'] == 'success': + return return_value['return'] + + module = importlib.import_module(return_value['exception']['module']) + exception_class = getattr(module, return_value['exception']['name']) + exception = exception_class(*return_value['exception']['args'], output, + error) + if log_error: + logger.error('Error running action %s..%s(*%s, **%s): %s %s %s', + module_name, action_name, args, kwargs, exception, + exception.args, return_value['exception']['traceback']) + + raise exception + + +def _thread_reader(read_fd, buffers): + """Read from the pipe in a separate thread.""" + while True: + buffer = os.read(read_fd, 10240) + if not buffer: + break + + buffers.append(buffer) + + os.close(read_fd) + + def _check_privileged_action_arguments(func): """Check that a privileged action has well defined types.""" argspec = inspect.getfullargspec(func) @@ -320,3 +414,10 @@ def _get_privileged_action_module_name(func): 'package/module named privileged') return module_name.rpartition('.')[2] + + +def _log_action(module_name, action_name, run_as_user, run_in_background): + """Log an action in a compact format.""" + prompt = f'({run_as_user})$' if run_as_user else '#' + suffix = '&' if run_in_background else '' + logger.info('%s %s..%s(…) %s', prompt, module_name, action_name, suffix) diff --git a/plinth/tests/test_actions.py b/plinth/tests/test_actions.py index 48baa6e31..ed76581a2 100644 --- a/plinth/tests/test_actions.py +++ b/plinth/tests/test_actions.py @@ -11,8 +11,9 @@ import json import os import pathlib import shutil +import subprocess import tempfile -from unittest.mock import call, patch +from unittest.mock import Mock, call, patch import pytest @@ -21,6 +22,28 @@ from plinth.actions import _log_command as log_command from plinth.actions import privileged, run, superuser_run +@pytest.fixture(name='popen') +def fixture_popen(): + """A fixture to patch subprocess.Popen called by privileged action.""" + + with patch('subprocess.Popen') as popen: + + def call_popen(command, **kwargs): + write_fd = int(command[8]) + if not isinstance(popen.called_with_write_fd, list): + popen.called_with_write_fd = [] + + popen.called_with_write_fd.append(write_fd) + os.write(write_fd, bytes(popen.return_value, encoding='utf-8')) + proc = Mock() + proc.communicate.return_value = ('', '') + proc.returncode = 0 + return proc + + popen.side_effect = call_popen + yield popen + + @pytest.fixture(autouse=True) def actions_test_setup(load_cfg): """Setup a temporary directory for testing actions. @@ -227,8 +250,7 @@ def test_privileged_argument_annotation_check(): @patch('plinth.actions._get_privileged_action_module_name') -@patch('plinth.actions.superuser_run') -def test_privileged_method_call(superuser_run_, get_module_name): +def test_privileged_method_call(get_module_name, popen): """Test that privileged method calls the superuser action properly.""" def func_with_args(_a: int, _b: str, _c: int = 1, _d: str = 'dval', @@ -236,35 +258,41 @@ def test_privileged_method_call(superuser_run_, get_module_name): return get_module_name.return_value = 'tests' - superuser_run_.return_value = json.dumps({ - 'result': 'success', - 'return': 'bar' - }) + popen.return_value = json.dumps({'result': 'success', 'return': 'bar'}) wrapped_func = privileged(func_with_args) return_value = wrapped_func(1, 'bval', None, _d='dnewval') assert return_value == 'bar' input_ = {'args': [1, 'bval', None], 'kwargs': {'_d': 'dnewval'}} input_ = json.dumps(input_) - superuser_run_.assert_has_calls( - [call('actions', ['tests', 'func_with_args'], input=input_.encode())]) + write_fd = popen.called_with_write_fd[0] + close_from_fd = str(write_fd + 1) + popen.assert_has_calls([ + call([ + 'sudo', '--non-interactive', '--close-from', close_from_fd, + cfg.actions_dir + '/actions', 'tests', 'func_with_args', + '--write-fd', + str(write_fd) + ], stdin=subprocess.PIPE, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, shell=False, pass_fds=[write_fd]) + ]) @patch('plinth.actions._get_privileged_action_module_name') -@patch('plinth.actions.superuser_run') -def test_privileged_method_exceptions(superuser_run_, get_module_name): +def test_privileged_method_exceptions(get_module_name, popen): """Test that exceptions on privileged methods are return properly.""" def func_with_exception(): raise TypeError('type error') get_module_name.return_value = 'tests' - superuser_run_.return_value = json.dumps({ + popen.return_value = json.dumps({ 'result': 'exception', 'exception': { 'module': 'builtins', 'name': 'TypeError', - 'args': ['type error'] + 'args': ['type error'], + 'traceback': [''] } }) wrapped_func = privileged(func_with_exception) diff --git a/plinth/tests/test_actions_actions.py b/plinth/tests/test_actions_actions.py index 9a5a84673..76802f111 100644 --- a/plinth/tests/test_actions_actions.py +++ b/plinth/tests/test_actions_actions.py @@ -77,14 +77,13 @@ def test_call_syntax_checks(getuid, get_module_import_path, import_module, setattr(module, 'func', exception_func) return_value = call('test-module', 'func', {'args': [], 'kwargs': {}}) - assert return_value == { - 'result': 'exception', - 'exception': { - 'module': 'builtins', - 'name': 'RuntimeError', - 'args': ('foo exception', ) - } - } + assert return_value['result'] == 'exception' + assert return_value['exception']['module'] == 'builtins' + assert return_value['exception']['name'] == 'RuntimeError' + assert return_value['exception']['args'] == ('foo exception', ) + assert isinstance(return_value['exception']['traceback'], list) + for line in return_value['exception']['traceback']: + assert isinstance(line, str) def test_assert_valid_arguments(actions_module): diff --git a/vagrant-scripts/plinth-user-permissions.py b/vagrant-scripts/plinth-user-permissions.py index e081446c3..64ade9b4e 100755 --- a/vagrant-scripts/plinth-user-permissions.py +++ b/vagrant-scripts/plinth-user-permissions.py @@ -1,24 +1,16 @@ #!/usr/bin/python3 # -*- mode: python -*- # SPDX-License-Identifier: AGPL-3.0-or-later -""" -Set required permissions for user "plinth" to run plinth in the development -environment. -""" +"""Set required permissions for user "plinth" to run plinth in dev setup.""" -import augeas +import pathlib -sudoers_file = '/etc/sudoers.d/plinth' -aug = augeas.Augeas(flags=augeas.Augeas.NO_LOAD + - augeas.Augeas.NO_MODL_AUTOLOAD) +content = ''' +Cmnd_Alias FREEDOMBOX_ACTION_DEV = /usr/share/plinth/actions/actions, /vagrant/actions/actions +Defaults!FREEDOMBOX_ACTION_DEV closefrom_override +plinth ALL=(ALL:ALL) NOPASSWD:SETENV : FREEDOMBOX_ACTION_DEV +fbx ALL=(ALL:ALL) NOPASSWD : ALL +''' -# lens for shell-script config file -aug.set('/augeas/load/Shellvars/lens', 'Sudoers.lns') -aug.set('/augeas/load/Shellvars/incl[last() + 1]', sudoers_file) -aug.load() - -aug.set('/files{}/spec[1]/host_group/command[2]'.format(sudoers_file), - '/vagrant/actions/*') -aug.set('/files{}/spec[1]/host_group/command[1]/tag[2]'.format(sudoers_file), - 'SETENV') -aug.save() +sudoers_file = pathlib.Path('/etc/sudoers.d/01-freedombox-development') +sudoers_file.write_text(content)