ssh: Use privileged decorator for actions

Tests:

- Functional tests work
- Updating SSH keys in user edit page sets the SSH keys. File is updated
  properly. Page shows newly set SSH keys.
  - Entering invalid auth credentials throws error
- Enabling/disabling SSH password authentication works.
  - Configuration file is updated.
  - App page shows proper value

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

plinth/modules/ssh/__init__.py

@@ -1,7 +1,5 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""FreedomBox app for OpenSSH server."""
-FreedomBox app for OpenSSH server.
-"""
 
 import pathlib
 import re
@@ -9,7 +7,6 @@ import subprocess
 
 from django.utils.translation import gettext_lazy as _
 
-from plinth import actions
 from plinth import app as app_module
 from plinth import menu
 from plinth.daemon import Daemon
@@ -17,7 +14,7 @@ from plinth.modules.backups.components import BackupRestore
 from plinth.modules.firewall.components import Firewall
 from plinth.package import Packages
 
-from . import manifest
+from . import manifest, privileged
 
 _description = [
     _('A Secure Shell server uses the secure shell protocol to accept '
@@ -65,7 +62,7 @@ class SSHApp(app_module.App):
     def setup(self, old_version):
         """Install and configure the app."""
         super().setup(old_version)
-        actions.superuser_run('ssh', ['setup'])
+        privileged.setup()
         self.enable()
 
 
@@ -87,9 +84,3 @@ def get_host_keys():
                 host_keys.append(match.groupdict())
 
     return host_keys
-
-
-def is_password_authentication_disabled():
-    """Return if ssh password authentication is enabled."""
-    return actions.superuser_run('ssh',
-                                 ['get-password-config']).strip() == 'no'

plinth/modules/ssh/privileged.py

@@ -1,52 +1,20 @@
-#!/usr/bin/python3
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""Configuration helper for SSH server."""
-Configuration helper for SSH server.
-"""
 
-import argparse
 import grp
 import os
 import pwd
 import shutil
 import stat
 import subprocess
-import sys
 
 import augeas
 
 from plinth import action_utils, utils
+from plinth.actions import privileged
 
 
-def parse_arguments():
+def _validate_user(username, password, must_be_admin=True):
-    """Return parsed command line arguments as dictionary."""
-    parser = argparse.ArgumentParser()
-    subparsers = parser.add_subparsers(dest='subcommand', help='Sub command')
-
-    subparsers.add_parser('setup', help='Setup SSH server')
-
-    get_keys = subparsers.add_parser('get-keys',
-                                     help='Get SSH authorized keys')
-    get_keys.add_argument('--username', required=True, type=_managed_user)
-
-    set_keys = subparsers.add_parser('set-keys',
-                                     help='Set SSH authorized keys')
-    set_keys.add_argument('--username', required=True, type=_managed_user)
-    set_keys.add_argument('--keys', required=True)
-    set_keys.add_argument('--auth-user', required=True)
-
-    subparsers.add_parser('get-password-config',
-                          help='Get SSH password auth configuration')
-
-    set_password_config = subparsers.add_parser(
-        'set-password-config', help='Set SSH password auth configuration')
-    set_password_config.add_argument('--value')
-
-    subparsers.required = True
-    return parser.parse_args()
-
-
-def _validate_user(username, must_be_admin=True):
     """Validate a user."""
     if must_be_admin:
         try:
@@ -55,29 +23,23 @@ def _validate_user(username, must_be_admin=True):
             admins = []
 
         if username not in admins:
-            msg = '"{}" is not authorized to perform this action'.format(
+            msg = f'"{username}" is not authorized to perform this action'
-                username)
+            raise PermissionError(msg)
-            raise argparse.ArgumentTypeError(msg)
 
-    password = _read_password()
     if not utils.is_authenticated_user(username, password):
-        raise argparse.ArgumentTypeError("Invalid credentials")
+        raise PermissionError('Invalid credentials')
 
 
 def _managed_user(username):
     """Raise an error if the user is root."""
     if pwd.getpwnam(username).pw_gid == 0:
-        msg = 'User {} is not managed by FreedomBox'.format(username)
+        raise ValueError(f'User {username} is not managed by FreedomBox')
-        raise argparse.ArgumentTypeError(msg)
+
     return username
 
 
-def _read_password():
+@privileged
-    """Read the password from stdin."""
+def setup():
-    return ''.join(sys.stdin)
-
-
-def subcommand_setup(arguments):
     """Setup Open SSH server.
 
     Regenerates deleted SSH keys. This is necessary when FreedomBox image is
@@ -87,7 +49,6 @@ def subcommand_setup(arguments):
     If the keys already exist, do nothing. This is necessary when a user
     installs FreedomBox using an apt package. SSH keys exist and running
     reconfigure on the openssh-server package does not regenerate them.
-
     """
     action_utils.dpkg_reconfigure('openssh-server', {})
 
@@ -97,29 +58,25 @@ def get_user_homedir(username):
     try:
         return pwd.getpwnam(username).pw_dir
     except KeyError:
-        print('Username not found')
+        raise ValueError('Username not found')
-        sys.exit(1)
 
 
-def subcommand_get_keys(arguments):
+@privileged
+def get_keys(user: str) -> str:
     """Get SSH authorized keys."""
-    user = arguments.username
-
     path = os.path.join(get_user_homedir(user), '.ssh', 'authorized_keys')
     try:
         with open(path, 'r', encoding='utf-8') as file_handle:
-            print(file_handle.read())
+            return file_handle.read()
     except FileNotFoundError:
-        pass
+        return ''
 
 
-def subcommand_set_keys(arguments):
+@privileged
+def set_keys(user: str, keys: str, auth_user: str, auth_password: str):
     """Set SSH authorized keys."""
-    user = arguments.username
-    auth_user = arguments.auth_user
-
     must_be_admin = user != auth_user
-    _validate_user(auth_user, must_be_admin=must_be_admin)
+    _validate_user(auth_user, auth_password, must_be_admin=must_be_admin)
 
     ssh_folder = os.path.join(get_user_homedir(user), '.ssh')
     key_file_path = os.path.join(ssh_folder, 'authorized_keys')
@@ -131,7 +88,7 @@ def subcommand_set_keys(arguments):
         shutil.chown(ssh_folder, user, 'users')
 
     with open(key_file_path, 'w', encoding='utf-8') as file_handle:
-        file_handle.write(arguments.keys)
+        file_handle.write(keys)
 
     shutil.chown(key_file_path, user, 'users')
     os.chmod(key_file_path, stat.S_IRUSR | stat.S_IWUSR)
@@ -148,30 +105,19 @@ def _load_augeas():
     return aug
 
 
-def subcommand_get_password_config(_):
+@privileged
+def is_password_authentication_enabled() -> bool:
     """Retrieve value of password authentication from sshd configuration."""
     aug = _load_augeas()
     field_path = '/files/etc/ssh/sshd_config/PasswordAuthentication'
     get_value = aug.get(field_path)
-    print(get_value or 'yes')
+    return (get_value or 'yes') == 'yes'
 
 
-def subcommand_set_password_config(arguments):
+@privileged
+def set_password_authentication(enable: bool):
     """Set value of password authentication in sshd configuration."""
+    value = 'yes' if enable else 'no'
     aug = _load_augeas()
-    aug.set('/files/etc/ssh/sshd_config/PasswordAuthentication',
+    aug.set('/files/etc/ssh/sshd_config/PasswordAuthentication', value)
-            arguments.value)
     aug.save()
-
-
-def main():
-    """Parse arguments and perform all duties."""
-    arguments = parse_arguments()
-
-    subcommand = arguments.subcommand.replace('-', '_')
-    subcommand_method = globals()['subcommand_' + subcommand]
-    subcommand_method(arguments)
-
-
-if __name__ == '__main__':
-    main()

plinth/modules/ssh/views.py

@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""Views for the SSH app."""
-Views for the SSH module
+
-"""
 from django.contrib import messages
 from django.utils.translation import gettext_lazy as _
 
@@ -9,32 +8,36 @@ from plinth import actions
 from plinth.modules import ssh
 from plinth.views import AppView
 
-from . import is_password_authentication_disabled
+from . import privileged
 from .forms import SSHServerForm
 
 
 class SshAppView(AppView):
+    """Show ssh app main page."""
+
     app_id = 'ssh'
     template_name = 'ssh.html'
     form_class = SSHServerForm
 
     def get_context_data(self, *args, **kwargs):
+        """Return additional context for rendering the template."""
         context = super().get_context_data(**kwargs)
         context['host_keys'] = ssh.get_host_keys()
 
         return context
 
     def get_initial(self):
-        """Initial form value"""
+        """Return initial values of the form."""
         initial = super().get_initial()
         initial.update({
-            'password_auth_disabled': is_password_authentication_disabled(),
+            'password_auth_disabled':
+                not privileged.is_password_authentication_enabled(),
         })
 
         return initial
 
     def form_valid(self, form):
-        """Apply changes from the form"""
+        """Apply changes from the form."""
         old_config = self.get_initial()
         new_config = form.cleaned_data
 
@@ -43,16 +46,9 @@ class SshAppView(AppView):
 
         passwd_auth_changed = is_field_changed('password_auth_disabled')
         if passwd_auth_changed:
-            if new_config['password_auth_disabled']:
+            privileged.set_password_authentication(
-                passwd_auth = 'no'
+                not new_config['password_auth_disabled'])
-                message = _('SSH authentication with password disabled.')
-            else:
-                passwd_auth = 'yes'
-                message = _('SSH authentication with password enabled.')
-
-            actions.superuser_run(
-                'ssh', ['set-password-config', '--value', passwd_auth])
             actions.superuser_run('service', ['reload', 'ssh'])
-            messages.success(self.request, message)
+            messages.success(self.request, _('Configuration updated'))
 
         return super().form_valid(form)

plinth/modules/users/forms.py

@@ -15,6 +15,7 @@ from django.utils.translation import gettext as _
 from django.utils.translation import gettext_lazy
 
 import plinth.forms
+import plinth.modules.ssh.privileged as ssh_privileged
 from plinth import actions
 from plinth.errors import ActionError
 from plinth.modules import first_boot
@@ -294,16 +295,10 @@ class UserUpdateForm(ValidNewUsernameCheckMixin, PasswordConfirmForm,
                                        _('Failed to add user to group.'))
 
             try:
-                actions.superuser_run('ssh', [
+                ssh_privileged.set_keys(user.get_username(),
-                    'set-keys',
+                                        self.cleaned_data['ssh_keys'].strip(),
-                    '--username',
+                                        auth_username, confirm_password)
-                    user.get_username(),
+            except Exception:
-                    '--keys',
-                    self.cleaned_data['ssh_keys'].strip(),
-                    '--auth-user',
-                    auth_username,
-                ], input=confirm_password.encode())
-            except ActionError:
                 messages.error(self.request, _('Unable to set SSH keys.'))
 
             is_active = self.cleaned_data['is_active']

plinth/modules/users/views.py

@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
+"""Django views for user app."""
 
 import django.views.generic
 from django.contrib import messages
@@ -13,6 +14,7 @@ from django.utils.translation import gettext_lazy
 from django.views.generic.edit import (CreateView, DeleteView, FormView,
                                        UpdateView)
 
+import plinth.modules.ssh.privileged as ssh_privileged
 from plinth import actions, translation
 from plinth.errors import ActionError
 from plinth.modules import first_boot
@@ -36,6 +38,7 @@ class ContextMixin:
 
 class UserCreate(ContextMixin, SuccessMessageMixin, CreateView):
     """View to create a new user."""
+
     form_class = CreateUserForm
     template_name = 'users_create.html'
     model = User
@@ -95,8 +98,7 @@ class UserUpdate(ContextMixin, SuccessMessageMixin, UpdateView):
         """Return the data for initial form load."""
         initial = super().get_initial()
         try:
-            ssh_keys = actions.superuser_run(
+            ssh_keys = ssh_privileged.get_keys(self.object.username)
-                'ssh', ['get-keys', '--username', self.object.username])
             initial['ssh_keys'] = ssh_keys.strip()
             initial['language'] = self.object.userprofile.language
         except ActionError: