From dd0a0f56a67f2a834158c82cb97248ec296e8804 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Thu, 5 Feb 2026 08:31:01 -0800 Subject: [PATCH] backups: Simplify handling of migration to SSH keys - Integrate SSH error handling into borg error handling. - Move logic to migrate SSH keys into lower levels (Repository class) so that it can performed at more instances such as when initializing repository. It also provides better abstraction keeping the view logic simpler. - Drop ability to mount repository using password. This is important next step for mounting using systemd unit files. - Use exceptions to eliminate return value checking. - Create a special exception for exceptions raised during SSH operations. Raise this at lower levels and handle these using the common error handler. Tests: - Adding a remote repository with key and password authentication works with and without encryption. Adding works with SSH host key pre-verified works too. - Trying to add a remote repository with incorrect passpharse fails with the simplified error message. Redirect happens to add remote repository page. Error message with SSH host key pre-verified works too. Repository is removed. - Trying to provide wrong SSH password fails with a simplified error message. Redirect happens to add remote repository page. Repository is removed. - Mounting a repository after unmounting it works. - Mounting a repository with SSH password in its configuration works. Migration is performed and SSH password is replaced with SSH key file path. - A schedule for a repository with SSH password runs successfully. An archive is created. Migration is performed and SSH password is replaced with SSH key file path. - SSH identity files are created with plinth:plinth ownership. Private key file is created with 0o600 permissions and public key file is created with 0o644 permissions. Signed-off-by: Sunil Mohan Adapa --- plinth/modules/backups/__init__.py | 40 +++++--- plinth/modules/backups/errors.py | 12 ++- plinth/modules/backups/privileged.py | 14 +-- plinth/modules/backups/repository.py | 48 +++++---- plinth/modules/backups/views.py | 142 +++++++-------------------- 5 files changed, 106 insertions(+), 150 deletions(-) diff --git a/plinth/modules/backups/__init__.py b/plinth/modules/backups/__init__.py index 5991cbaaf..48aeb9900 100644 --- a/plinth/modules/backups/__init__.py +++ b/plinth/modules/backups/__init__.py @@ -1,12 +1,13 @@ # SPDX-License-Identifier: AGPL-3.0-or-later """FreedomBox app to manage backup archives.""" +import contextlib import json import logging import os -from pathlib import Path import re import subprocess +from pathlib import Path from django.utils.text import get_valid_filename from django.utils.translation import gettext_lazy as _ @@ -16,7 +17,7 @@ from plinth import app as app_module from plinth import cfg, glib, menu from plinth.package import Packages -from . import api, manifest, privileged +from . import api, errors, manifest, privileged logger = logging.getLogger(__name__) @@ -167,21 +168,38 @@ def get_ssh_client_public_key() -> str: return pubkey -def copy_ssh_client_public_key(hostname: str, username: str, - password: str) -> tuple[bool, str]: +def copy_ssh_client_public_key(pubkey_path: str, hostname: str, username: str, + password: str): """Copy the SSH client public key to the remote server. Returns whether the copy was successful, and any error message. """ pubkey_path, _ = get_ssh_client_auth_key_paths() env = os.environ.copy() - env['SSHPASS'] = password - process = subprocess.run([ - 'sshpass', '-e', 'ssh-copy-id', '-i', - str(pubkey_path), f'{username}@{hostname}' - ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=False, env=env) - error_message = process.stderr.decode() if process.returncode else '' - return (process.returncode == 0, error_message) + env['SSHPASS'] = str(password) + with raise_ssh_error(): + try: + subprocess.run([ + 'sshpass', '-e', 'ssh-copy-id', '-i', + str(pubkey_path), f'{username}@{hostname}' + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True, + env=env) + logger.info("Copied SSH client public key to remote host's " + "authorized keys.") + except subprocess.CalledProcessError as exception: + logger.warning('Failed to copy SSH client public key: %s', + exception.stderr) + raise + + +@contextlib.contextmanager +def raise_ssh_error() -> None: + """Convert subprocess error to SshError.""" + try: + yield + except subprocess.CalledProcessError as exception: + raise errors.SshError(exception.returncode, exception.cmd, + exception.output, exception.stderr) def is_ssh_hostkey_verified(hostname): diff --git a/plinth/modules/backups/errors.py b/plinth/modules/backups/errors.py index 2792f6741..d2cd56358 100644 --- a/plinth/modules/backups/errors.py +++ b/plinth/modules/backups/errors.py @@ -1,18 +1,24 @@ # SPDX-License-Identifier: AGPL-3.0-or-later +import subprocess + from plinth.errors import PlinthError class BorgError(PlinthError): - """Generic borg errors""" + """Generic borg errors.""" class BorgRepositoryDoesNotExistError(BorgError): - """Borg access to a repository works but the repository does not exist""" + """Borg access to a repository works but the repository does not exist.""" + + +class SshError(subprocess.CalledProcessError): + """Error when running an SSH command.""" class SshfsError(PlinthError): - """Generic sshfs errors""" + """Generic sshfs errors.""" class BorgRepositoryExists(BorgError): diff --git a/plinth/modules/backups/privileged.py b/plinth/modules/backups/privileged.py index 78d49223b..9d0a9d4a0 100644 --- a/plinth/modules/backups/privileged.py +++ b/plinth/modules/backups/privileged.py @@ -131,8 +131,7 @@ def _reraise_known_errors(err): @reraise_known_errors @privileged -def mount(mountpoint: str, remote_path: str, ssh_keyfile: str | None = None, - password: secret_str | None = None, +def mount(mountpoint: str, remote_path: str, ssh_keyfile: str, user_known_hosts_file: str = '/dev/null'): """Mount a remote ssh path via sshfs.""" try: @@ -156,16 +155,9 @@ def mount(mountpoint: str, remote_path: str, ssh_keyfile: str | None = None, 'sshfs', remote_path, mountpoint, '-o', f'UserKnownHostsFile={user_known_hosts_file}', '-o', 'StrictHostKeyChecking=yes', '-o', 'reconnect', '-o', - 'ServerAliveInterval=15', '-o', 'ServerAliveCountMax=3' + 'ServerAliveInterval=15', '-o', 'ServerAliveCountMax=3', '-o', + 'IdentityFile=' + ssh_keyfile ] - if ssh_keyfile: - cmd += ['-o', 'IdentityFile=' + ssh_keyfile] - else: - if not password: - raise ValueError('mount requires either a password or ssh_keyfile') - cmd += ['-o', 'password_stdin'] - input_ = password.encode() - action_utils.run(cmd, check=True, timeout=TIMEOUT, input=input_) diff --git a/plinth/modules/backups/repository.py b/plinth/modules/backups/repository.py index 371e22730..89b64b26d 100644 --- a/plinth/modules/backups/repository.py +++ b/plinth/modules/backups/repository.py @@ -13,8 +13,9 @@ from django.utils.translation import gettext_lazy as _ from plinth import cfg from plinth.utils import format_lazy -from . import (_backup_handler, api, errors, get_known_hosts_path, - get_ssh_client_auth_key_paths, privileged, +from . import (_backup_handler, api, copy_ssh_client_public_key, errors, + generate_ssh_client_auth_key, get_known_hosts_path, + get_ssh_client_auth_key_paths, privileged, raise_ssh_error, restore_archive_handler, split_path, store) from .schedule import Schedule @@ -144,6 +145,9 @@ class BaseBorgRepository(abc.ABC): privileged.delete_archive(archive_path, self._get_encryption_passpharse()) + def migrate_credentials(self) -> None: + """Migrate any credentials.""" + def initialize(self): """Initialize / create a borg repository.""" encryption = 'none' @@ -347,14 +351,22 @@ class SshBorgRepository(BaseBorgRepository): """Return whether remote path is mounted locally.""" return privileged.is_mounted(self._mountpoint) - def replace_ssh_password_with_keyfile(self, keyfile_path: str): + def migrate_credentials(self) -> None: """Add SSH keyfile credential and delete stored password.""" - self.credentials['ssh_keyfile'] = keyfile_path + if not self.ssh_password: + return + + pubkey_path, keyfile_path = get_ssh_client_auth_key_paths() + generate_ssh_client_auth_key() + copy_ssh_client_public_key(str(pubkey_path), self.hostname, + self.username, self.ssh_password) + self.credentials['ssh_keyfile'] = str(keyfile_path) self.credentials.pop('ssh_password', None) self.save() def initialize(self): """Initialize the repository after mounting the target directory.""" + self.migrate_credentials() self._ensure_remote_directory() self.mount() super().initialize() @@ -364,17 +376,11 @@ class SshBorgRepository(BaseBorgRepository): if self.is_mounted: return + self.migrate_credentials() known_hosts_path = get_known_hosts_path() - kwargs = {'user_known_hosts_file': str(known_hosts_path)} - if 'ssh_password' in self.credentials and self.credentials[ - 'ssh_password']: - kwargs['password'] = self.credentials['ssh_password'] - - if 'ssh_keyfile' in self.credentials and self.credentials[ - 'ssh_keyfile']: - kwargs['ssh_keyfile'] = self.credentials['ssh_keyfile'] - - privileged.mount(self._mountpoint, self._path, **kwargs) + privileged.mount(self._mountpoint, self._path, + ssh_keyfile=self.credentials['ssh_keyfile'], + user_known_hosts_file=str(known_hosts_path)) def umount(self): """Unmount the remote path that was mounted locally using sshfs.""" @@ -415,13 +421,15 @@ class SshBorgRepository(BaseBorgRepository): dir_path = '.' + dir_path[1:] # Ensure remote directory exists, check contents - _, key_path = get_ssh_client_auth_key_paths() known_hosts_path = str(get_known_hosts_path()) - subprocess.run([ - 'ssh', '-i', - str(key_path), '-o', f'UserKnownHostsFile={known_hosts_path}', - f'{username}@{hostname}', 'mkdir', '-p', dir_path - ], check=True) + with raise_ssh_error(): + subprocess.run([ + 'ssh', '-i', + str(self.ssh_keyfile), '-o', + f'UserKnownHostsFile={known_hosts_path}', '-o', + 'BatchMode=yes', f'{username}@{hostname}', 'mkdir', '-p', + dir_path + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True) def get_repositories(): diff --git a/plinth/modules/backups/views.py b/plinth/modules/backups/views.py index 02491871b..36accb365 100644 --- a/plinth/modules/backups/views.py +++ b/plinth/modules/backups/views.py @@ -6,7 +6,6 @@ Views for the backups app. import contextlib import logging import os -import subprocess from urllib.parse import unquote from django.contrib import messages @@ -24,8 +23,8 @@ from plinth.errors import PlinthError from plinth.modules import backups, storage from plinth.views import AppView -from . import (SESSION_PATH_VARIABLE, api, copy_ssh_client_public_key, errors, - forms, generate_ssh_client_auth_key, get_known_hosts_path, +from . import (SESSION_PATH_VARIABLE, api, errors, forms, + generate_ssh_client_auth_key, get_known_hosts_path, get_ssh_client_auth_key_paths, get_ssh_client_public_key, is_ssh_hostkey_verified, privileged) from .decorators import delete_tmp_backup_file @@ -40,6 +39,19 @@ def handle_common_errors(request: HttpRequest): """If any known Borg exceptions occur, show proper error messages.""" try: yield + except errors.SshError as exception: + if exception.returncode in (6, 7): + message = _('SSH host public key could not be verified.') + elif (exception.returncode == 5 + or 'Permission denied' in exception.stderr.decode()): + message = _('Authentication to remote server failed.') + else: + message = _( + 'Error establishing connection to server: {} {} {}').format( + str(exception), exception.stdout.decode(), + exception.stderr.decode()) + + messages.error(request, message) except errors.BorgError as exception: messages.error(request, exception.args[0]) @@ -346,11 +358,11 @@ class AddRepositoryView(FormView): encryption_passphrase = None credentials = {'encryption_passphrase': encryption_passphrase} + repository = BorgRepository(path, credentials) with handle_common_errors(self.request): - repository = BorgRepository(path, credentials) - if _save_repository(self.request, repository): - messages.success(self.request, _('Added new repository.')) - return super().form_valid(form) + _save_repository(self.request, repository) + messages.success(self.request, _('Added new repository.')) + return super().form_valid(form) return redirect(reverse_lazy('backups:add-repository')) @@ -440,79 +452,28 @@ class VerifySshHostkeyView(FormView): with known_hosts_path.open('a', encoding='utf-8') as known_hosts_file: known_hosts_file.write(ssh_public_key + '\n') - def _check_save_repository(self): + def _save_repository_and_redirect(self): """Save the repository and redirect according to the result.""" - if _save_repository(self.request, self._get_repository()): + with handle_common_errors(self.request): + _save_repository(self.request, self._get_repository()) return redirect(reverse_lazy('backups:index')) return redirect(reverse_lazy('backups:add-remote-repository')) - def _check_copy_ssh_client_public_key(self): - """Try to copy FreedomBox's SSH client public key to the host.""" - repo = self._get_repository() - ssh_password = repo.ssh_password - if ssh_password: - result, message = copy_ssh_client_public_key( - repo.hostname, repo.username, repo.ssh_password) - if result: - logger.info("Copied SSH client public key to remote host's " - "authorized keys.") - _pubkey_path, key_path = get_ssh_client_auth_key_paths() - repo.replace_ssh_password_with_keyfile(str(key_path)) - return self._check_save_repository() - - logger.warning('Failed to copy SSH client public key: %s', message) - messages.error( - self.request, - _('Failed to copy SSH client public key: %s') % message) - - else: - logger.error( - 'SSH password is required to copy SSH client public key.') - messages.error( - self.request, - _('SSH password is required to copy SSH public key.')) - - # Remove the repository so that the user can have another go at - # creating it. - try: - repo.remove() - messages.error(self.request, _('Repository removed.')) - except KeyError: - pass - - return redirect(reverse_lazy('backups:add-remote-repository')) - - def _check_next_step(self): - """Check whether we need to copy the SSH client public key. - - Otherwise, save the repository and redirect. - """ - if self._get_repository().ssh_keyfile: - # SSH keyfile credential is stored. Assume it is already copied to - # the remote host. Check the connection. - logger.info('Check connection using SSH keyfile...') - return self._check_save_repository() - - logger.info('Copy SSH client public key to remote host...') - return self._check_copy_ssh_client_public_key() - def get(self, *args, **kwargs): """Skip this view if host is already verified.""" if not is_ssh_hostkey_verified(self._get_repository().hostname): return super().get(*args, **kwargs) messages.success(self.request, _('SSH host already verified.')) - return self._check_next_step() + return self._save_repository_and_redirect() def form_valid(self, form): """Create and store the repository.""" ssh_public_key = form.cleaned_data['ssh_public_key'] - with handle_common_errors(self.request): - self._add_ssh_hostkey(ssh_public_key) - messages.success(self.request, _('SSH host verified.')) - - return self._check_next_step() + self._add_ssh_hostkey(ssh_public_key) + messages.success(self.request, _('SSH host verified.')) + return self._save_repository_and_redirect() def _save_repository(request, repository): @@ -521,29 +482,16 @@ def _save_repository(request, repository): repository.initialize() repository.verified = True repository.save() - return True - except subprocess.CalledProcessError as exception: - if exception.returncode in (6, 7): - message = _('SSH host public key could not be verified.') - elif exception.returncode == 5: - message = _('Authentication to remote server failed.') - else: - message = _('Error establishing connection to server: {}').format( - str(exception)) - except Exception as exception: - message = str(exception) - logger.exception('Error adding repository: %s', exception) + except Exception: + # Remove the repository so that the user can have another go at + # creating it. + try: + repository.remove() + messages.error(request, _('Repository removed.')) + except KeyError: + pass - messages.error(request, message) - # Remove the repository so that the user can have another go at - # creating it. - try: - repository.remove() - messages.error(request, _('Repository removed.')) - except KeyError: - pass - - return False + raise class RemoveRepositoryView(TemplateView): @@ -587,25 +535,9 @@ def mount_repository(request, uuid): return redirect('backups:verify-ssh-hostkey', uuid=uuid) repository = SshBorgRepository.load(uuid) - if repository.ssh_password: - logger.info('Migrating from SSH password to key authentication...') - generate_ssh_client_auth_key() - result, message = copy_ssh_client_public_key(repository.hostname, - repository.username, - repository.ssh_password) - if result: - logger.info("Copied SSH client public key to remote host's " - "authorized keys.") - _pubkey_path, key_path = get_ssh_client_auth_key_paths() - repository.replace_ssh_password_with_keyfile(str(key_path)) - else: - logger.warning('Failed to copy SSH client public key: %s', message) - messages.error( - request, - _('Failed to copy SSH client public key: %s') % message) - try: - repository.mount() + with handle_common_errors(request): + repository.mount() except Exception as err: msg = "%s: %s" % (_('Mounting failed'), str(err)) messages.error(request, msg)