diff --git a/conftest.py b/conftest.py index 429743bef..3ba2576e8 100644 --- a/conftest.py +++ b/conftest.py @@ -148,26 +148,6 @@ def fixture_actions_module(request): return module -@pytest.fixture(name='call_action') -def fixture_call_action(request, capsys, actions_module): - """Run actions with custom root path.""" - - actions_name = getattr(request.module, 'actions_name') - - def _call_action(*args, **_kwargs): - if isinstance(args[0], list): - argv = [actions_name] + args[0] # Command line style usage - else: - argv = [args[0]] + args[1] # superuser_run style usage - - with patch('argparse._sys.argv', argv): - actions_module.main() - captured = capsys.readouterr() - return captured.out - - return _call_action - - @pytest.fixture(name='mock_privileged') def fixture_mock_privileged(request): """Mock the privileged decorator to nullify its effects.""" diff --git a/doc/dev/reference/actions.rst b/doc/dev/reference/actions.rst index 53f21c8cd..4ace1950c 100644 --- a/doc/dev/reference/actions.rst +++ b/doc/dev/reference/actions.rst @@ -13,4 +13,4 @@ else. These actions are also directly usable by a skilled administrator. The following documentation for the ``actions`` module. .. automodule:: plinth.actions - :members: run, superuser_run, run_as_user, _run, privileged + :members: privileged diff --git a/plinth/actions.py b/plinth/actions.py index 4c07205bb..b98eebab2 100644 --- a/plinth/actions.py +++ b/plinth/actions.py @@ -1,79 +1,5 @@ # SPDX-License-Identifier: AGPL-3.0-or-later -"""Run specified actions. - -Actions run commands with this contract (version 1.1): - -1. (promise) Super-user actions run as root. Normal actions do not. - -2. (promise) The actions directory can't be changed at run time. - - This guarantees that we can only select from the correct set of actions. - -3. (restriction) Only specifically allowed actions can run. - - A. Scripts in a directory above the actions directory can't be run. - - Arguments (and options) can't coerce the system to run actions in - directories above the actions directory. - - Arguments that fail this validation will raise a ValueError. - - B. Scripts in a directory beneath the actions directory can't be run. - - Arguments (and options) can't coerce the system to run actions in - sub-directories of the actions directory. - - (An important side-effect of this is that the system will not try to - follow symlinks to other action directories.) - - Arguments that fail this validation will raise a ValueError. - - C. Only one action can be called at a time. - - This prevents us from appending multiple (unexpected) actions to - the call. Any special shell characters in the action name will - simply be treated as the action itself when trying to search for - an action. The action will then not be found. - - $ action="echo '$options'; echo 'oops'" - $ options="hi" - $ $action # oops, the file system is gone! - - Arguments that fail this validation will raise a ValueError. - - D. Options can't be used to run other actions: - - $ action="echo '$options'" - $ options="hi'; rm -rf /;'" - $ $action # oops, the file system is gone! - - Any option that tries to include special shell characters will - simply be treated as an option with special characters and will - not be interpreted by the shell. - - Any call wishing to include special shell characters in options - list does not need to escape them. They are taken care of. The - option string is passed to the action exactly as it is passed in. - - E. Actions must exist in the actions directory. - -4. (promise) Options are passed as arguments to the action. - - Options can be provided as a list or as a tuple. - -5. (promise) Output is returned from the command. In case of an - error, ActionError is raised with action, output and error strings - as arguments. - -6. (limitation) Providing the process with input is not possible. - - Don't expect to give the process additional input after it's started. Any - interaction with the spawned process must be carried out through some other - method (maybe the process opens a socket, or something). - -7. Option - -""" +"""Framework to run specified actions with elevated privileges.""" import functools import importlib @@ -81,168 +7,14 @@ import inspect import json import logging import os -import re -import shlex import subprocess import threading from plinth import cfg -from plinth.errors import ActionError logger = logging.getLogger(__name__) -def run(action, options=None, input=None, run_in_background=False): - """Safely run a specific action as the current user. - - See actions._run for more information. - """ - return _run(action, options, input, run_in_background, False) - - -def superuser_run(action, options=None, input=None, run_in_background=False, - log_error=True): - """Safely run a specific action as root. - - See actions._run for more information. - """ - return _run(action, options, input, run_in_background, True, - log_error=log_error) - - -def run_as_user(action, options=None, input=None, run_in_background=False, - become_user=None): - """Run a command as a different user. - - If become_user is None, run as current user. - """ - return _run(action, options, input, run_in_background, False, become_user) - - -def _run(action, options=None, input=None, run_in_background=False, - run_as_root=False, become_user=None, log_error=True): - """Safely run a specific action as a normal user or root. - - Actions are pulled from the actions directory. - - - options are added to the action command. - - - input: data (as bytes) that will be sent to the action command's stdin. - - - run_in_background: run asynchronously or wait for the command to - complete. - - - run_as_root: execute the command through sudo. - - """ - if options is None: - options = [] - - # Contract 3A and 3B: don't call anything outside of the actions directory. - if os.sep in action: - raise ValueError('Action cannot contain: ' + os.sep) - - cmd = os.path.join(cfg.actions_dir, action) - if not os.path.realpath(cmd).startswith(cfg.actions_dir): - raise ValueError('Action has to be in directory %s' % cfg.actions_dir) - - # Contract 3C: interpret shell escape sequences as literal file names. - # Contract 3E: fail if the action doesn't exist or exists elsewhere. - if not os.access(cmd, os.F_OK): - raise ValueError('Action must exist in action directory.') - - cmd = [cmd] - - # Contract: 3C, 3D: don't allow shell special characters in - # options be interpreted by the shell. When using - # subprocess.Popen with list invocation and not shell invocation, - # escaping is unnecessary as each argument is passed directly to - # the command and not parsed by a shell. - if options: - if not isinstance(options, (list, tuple)): - raise ValueError('Options must be list or tuple.') - - cmd += list(options) # No escaping necessary - - # Contract 1: commands can run via sudo. - sudo_call = [] - if run_as_root: - sudo_call = ['sudo', '-n'] - elif become_user: - sudo_call = ['sudo', '-n', '-u', become_user] - - if cfg.develop and sudo_call: - # Passing 'env' does not work with sudo, so append the PYTHONPATH - # as part of the command - sudo_call += ['PYTHONPATH=%s' % cfg.file_root] - - if sudo_call: - cmd = sudo_call + cmd - - _log_command(cmd) - - # Contract 3C: don't interpret shell escape sequences. - # Contract 5 (and 6-ish). - kwargs = { - 'stdin': subprocess.PIPE, - 'stdout': subprocess.PIPE, - 'stderr': subprocess.PIPE, - 'shell': False, - } - if cfg.develop: - # In development mode pass on local pythonpath to access Plinth - kwargs['env'] = {'PYTHONPATH': cfg.file_root} - - proc = subprocess.Popen(cmd, **kwargs) - - if not run_in_background: - output, error = proc.communicate(input=input) - output, error = output.decode(), error.decode() - if proc.returncode != 0: - if log_error: - logger.error('Error executing command - %s, %s, %s', cmd, - output, error) - raise ActionError(action, output, error) - - return output - - return proc - - -def _log_command(cmd): - """Log a command with special pretty formatting to catch the eye.""" - cmd = list(cmd) # Make a copy of the command not to affect the original - - prompt = '$' - user = '' - if cmd and cmd[0] == 'sudo': - cmd = cmd[1:] - prompt = '#' - - # Drop -n argument to sudo - if cmd and cmd[0] == '-n': - cmd = cmd[1:] - - # Capture username separately - if len(cmd) > 1 and cmd[0] == '-u': - prompt = '$' - user = cmd[1] - cmd = cmd[2:] - - # Drop environmental variables set via sudo - while cmd and re.match(r'.*=.*', cmd[0]): - cmd = cmd[1:] - - # Strip the command's prefix - if cmd: - cmd[0] = cmd[0].split('/')[-1] - - # Shell escape and join command arguments - cmd = ' '.join([shlex.quote(part) for part in cmd]) - - logger.info('%s%s %s', user, prompt, cmd) - - def privileged(func): """Mark a method as allowed to be run as privileged method. diff --git a/plinth/errors.py b/plinth/errors.py index 32d4387f8..4c6bcaeba 100644 --- a/plinth/errors.py +++ b/plinth/errors.py @@ -8,10 +8,6 @@ class PlinthError(Exception): """Base class for all FreedomBox specific errors.""" -class ActionError(PlinthError): - """Use this error for exceptions when executing an action.""" - - class PackageNotInstalledError(PlinthError): """Could not complete module setup due to missing package.""" diff --git a/plinth/tests/test_actions.py b/plinth/tests/test_actions.py index ed76581a2..ed37a750e 100644 --- a/plinth/tests/test_actions.py +++ b/plinth/tests/test_actions.py @@ -9,17 +9,13 @@ description of the expectations. import json import os -import pathlib -import shutil import subprocess -import tempfile from unittest.mock import Mock, call, patch import pytest from plinth import cfg -from plinth.actions import _log_command as log_command -from plinth.actions import privileged, run, superuser_run +from plinth.actions import privileged @pytest.fixture(name='popen') @@ -44,160 +40,6 @@ def fixture_popen(): yield popen -@pytest.fixture(autouse=True) -def actions_test_setup(load_cfg): - """Setup a temporary directory for testing actions. - - Copy system commands ``echo`` and ``id`` into actions directory during - testing. - - """ - with tempfile.TemporaryDirectory() as tmp_directory: - old_actions_dir = cfg.actions_dir - cfg.actions_dir = str(tmp_directory) - - actions_dir = pathlib.Path(__file__).parent / '../../actions' - shutil.copy(str(actions_dir / 'test_path'), str(tmp_directory)) - shutil.copy('/bin/echo', str(tmp_directory)) - shutil.copy('/usr/bin/id', str(tmp_directory)) - - yield - cfg.actions_dir = old_actions_dir - - -def notest_run_as_root(): - """1. Privileged actions run as root. """ - assert superuser_run('id', ['-ur'])[0].strip() == '0' # user 0 is root - - -def test_breakout_actions_dir(): - """2. The actions directory can't be changed at run time. - - Can't currently be tested, as the actions directory is hardcoded. - """ - - -def test_breakout_up(): - """3A. Users can't call actions above the actions directory. - - Tests both a relative and a literal path. - """ - for action in ('../echo', '/bin/echo'): - with pytest.raises(ValueError): - run(action, ['hi']) - - -def test_breakout_down(): - """3B. Users can't call actions beneath the actions directory.""" - action = 'directory/echo' - with pytest.raises(ValueError): - superuser_run(action) - - -def test_breakout_actions(): - """3C. Actions can't be used to run other actions. - - If multiple actions are specified, bail out. - """ - # Counting is safer than actual badness. - actions = ('echo ""; echo $((1+1))', 'echo "" && echo $((1+1))', - 'echo "" || echo $((1+1))') - options = ('good', '') - - for action in actions: - for option in options: - with pytest.raises(ValueError): - run(action, [option]) - - -def test_breakout_option_string(): - """3D. Option strings can't be used to run other actions. - - Verify that shell control characters aren't interpreted. - """ - options = ('; echo hello', '&& echo hello', '|| echo hello', - '& echo hello', r'\; echo hello', '| echo hello', - r':;!&\/$%@`"~#*(){}[]|+=') - for option in options: - output = run('echo', [option]) - output = output.rstrip('\n') - assert option == output - - -def test_breakout_option_list(): - """3D. Option lists can't be used to run other actions. - - Verify that shell control characters aren't interpreted in - option lists. - """ - option_lists = ( - (';', 'echo', 'hello'), - ('&&', 'echo', 'hello'), - ('||', 'echo', 'hello'), - ('&', 'echo', 'hello'), - (r'\;', 'echo' - 'hello'), - ('|', 'echo', 'hello'), - ('', 'echo', '', 'hello'), # Empty option argument - tuple(r':;!&\/$%@`"~#*(){}[]|+=')) - for options in option_lists: - output = run('echo', options) - output = output.rstrip('\n') - expected_output = ' '.join(options) - assert output == expected_output - - -def test_multiple_options_and_output(): - """4. Multiple options can be provided as a list or as a tuple. - - 5. Output is returned from the command. - """ - options = '1 2 3 4 5 6 7 8 9' - - output = run('echo', options.split()) - output = output.rstrip('\n') - assert options == output - - output = run('echo', tuple(options.split())) - output = output.rstrip('\n') - assert options == output - - -@pytest.mark.usefixtures('develop_mode', 'needs_root') -def test_action_path(monkeypatch): - """Test that in development mode, python action scripts get the - correct PYTHONPATH""" - monkeypatch.setitem(os.environ, 'PYTHONPATH', '') - plinth_path = run('test_path').strip() - su_plinth_path = superuser_run('test_path').strip() - assert plinth_path.startswith(cfg.file_root) - assert plinth_path == su_plinth_path - - -@patch('plinth.actions.logger.info') -@pytest.mark.parametrize('cmd,message', [ - [['ls'], '$ ls'], - [['/bin/ls'], '$ ls'], - [['ls', 'a', 'b', 'c'], '$ ls a b c'], - [['ls', 'a b c'], "$ ls 'a b c'"], - [['ls', 'a;'], "$ ls 'a;'"], - [['sudo', 'ls'], '# ls'], - [['sudo', '-n', 'ls'], '# ls'], - [['sudo', '-u', 'tester', 'ls'], 'tester$ ls'], - [['sudo', 'key1=value1', 'key2=value2', 'ls'], '# ls'], - [[ - 'sudo', '-n', 'PYTHONPATH=/vagrant', '/vagrant/actions/ejabberd', - 'add-domain', '--domainname', 'freedombox.local' - ], '# ejabberd add-domain --domainname freedombox.local'], -]) -def test_log_command(logger, cmd, message): - """Test log messages for various action calls.""" - log_command(cmd) - logger.assert_called_once() - args = logger.call_args[0] - assert message == args[0] % args[1:] - - def test_privileged_properties(): """Test that privileged decorator sets proper properties on the method."""