actions: Use separate IPC for communicating results

Currently privileged actions use stdout for returning the results. If any of the
sub-processes accidentally output to stdout, decoding errors occur. Prevent this
by opening a pipe to the privileged action and returning the output in that
pipe.

Tests:

- Run unit tests
- Functional tests for other apps pass

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

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__)
             }
         }
 

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)

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

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)

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)

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):

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)