diff --git a/backend/app/services/firewall_service.py b/backend/app/services/firewall_service.py index 1842aca5..1b6980c1 100644 --- a/backend/app/services/firewall_service.py +++ b/backend/app/services/firewall_service.py @@ -46,43 +46,60 @@ class FirewallService: """Check firewalld status.""" try: installed = PackageManager.is_installed('firewalld') or is_command_available('firewall-cmd') + except Exception: + installed = False - running = False - default_zone = None - - if installed: + # Same separation as _check_ufw: a probe that fails must not be able to + # unsay an `installed` that was already determined correctly. + running = False + default_zone = None + if installed: + try: result = run_privileged(['firewall-cmd', '--state']) - running = 'running' in result.stdout.lower() + running = 'running' in (result.stdout or '').lower() if running: result = run_privileged(['firewall-cmd', '--get-default-zone']) - default_zone = result.stdout.strip() + default_zone = (result.stdout or '').strip() + except Exception: + running = False + default_zone = None - return { - 'installed': installed, - 'running': running, - 'default_zone': default_zone - } - except Exception: - return {'installed': False, 'running': False, 'default_zone': None} + return { + 'installed': installed, + 'running': running, + 'default_zone': default_zone + } @classmethod def _check_ufw(cls) -> Dict: - """Check ufw status.""" + """Check ufw status. + + The two questions are answered independently on purpose. A single + try/except around both let a failure in the *status* probe discard an + ``installed`` that had already been determined correctly, so a working + install was reported as "No Firewall Installed" — the probe raised + FileNotFoundError because ufw lives in /usr/sbin, which is absent from + the panel unit's PATH. Not knowing whether it is running says nothing + about whether it is there. + """ try: installed = PackageManager.is_installed('ufw') or is_command_available('ufw') - - active = False - if installed: - result = run_privileged(['ufw', 'status']) - active = 'Status: active' in result.stdout - - return { - 'installed': installed, - 'active': active - } except Exception: - return {'installed': False, 'active': False} + installed = False + + active = False + if installed: + try: + result = run_privileged(['ufw', 'status']) + active = 'Status: active' in (result.stdout or '') + except Exception: + active = False + + return { + 'installed': installed, + 'active': active + } @classmethod def enable(cls, firewall: str = None) -> Dict: diff --git a/backend/app/utils/system.py b/backend/app/utils/system.py index 691ff771..1182dbfd 100644 --- a/backend/app/utils/system.py +++ b/backend/app/utils/system.py @@ -50,6 +50,19 @@ def privileged_cmd(cmd: Union[List[str], str], *, user: Optional[str] = None) -> return cmd cmd = list(cmd) + + # Give argv[0] an absolute path ONLY when $PATH cannot resolve it. subprocess + # searches $PATH and nothing else, so a tool that exists but sits outside it + # raises FileNotFoundError — which callers wrapping the probe in a broad + # `except` then report as "not installed". This is how a successfully + # installed ufw (in /usr/sbin, absent from the unit's PATH) surfaced in the + # panel as "No Firewall Installed". Left untouched when $PATH already works, + # so a resolvable command is passed through exactly as the caller wrote it. + if cmd and cmd[0] != 'sudo' and not os.path.isabs(cmd[0]) and not shutil.which(cmd[0]): + resolved = resolve_command(cmd[0]) + if resolved: + cmd[0] = resolved + if _needs_sudo() and cmd[0] != 'sudo': if user: return ['sudo', '-n', '-u', user] + cmd @@ -108,23 +121,47 @@ def run_command(cmd: Union[List[str], str], *, timeout: int = 60, } +# Searched when ``$PATH`` does not resolve a command. Ordered the way a login +# shell would. ``/usr/sbin`` and ``/sbin`` matter most: the panel's systemd unit +# ships a PATH of venv:/usr/local/bin:/usr/bin:/bin, so every sbin-resident tool +# (ufw, iptables, nft) is unreachable by bare name from the service. ``/snap/bin`` +# is included because snap is the recommended install route for certbot and is +# almost never on a service's PATH. +_COMMAND_SEARCH_DIRS = ('/usr/local/sbin', '/usr/local/bin', '/usr/sbin', + '/usr/bin', '/sbin', '/bin', '/snap/bin') + + +def resolve_command(cmd: str) -> Optional[str]: + """Absolute path to *cmd*, or None when it cannot be found anywhere. + + ``shutil.which`` first, then :data:`_COMMAND_SEARCH_DIRS`. Knowing a command + exists is not enough to run it: ``subprocess`` resolves argv[0] through + ``$PATH`` alone and raises ``FileNotFoundError`` when that misses, so a + caller that only checked availability would still fail to exec. + """ + if os.path.isabs(cmd): + return cmd if os.path.exists(cmd) else None + + found = shutil.which(cmd) + if found: + return found + + for directory in _COMMAND_SEARCH_DIRS: + candidate = os.path.join(directory, cmd) + if os.path.exists(candidate): + return candidate + + return None + + def is_command_available(cmd: str) -> bool: """Check whether *cmd* is available on the system. - Uses ``shutil.which`` first, then falls back to checking common sbin/local - paths that may not be on the current ``$PATH``. ``/snap/bin`` is included - because snap is the recommended install method for tools like certbot, and - snap's bin dir is frequently missing from a service's ``$PATH``. + Uses ``shutil.which`` first, then falls back to the same sbin/local paths + :func:`resolve_command` searches — so "available" and "runnable" can never + disagree. """ - if shutil.which(cmd): - return True - - for directory in ('/usr/bin', '/usr/sbin', '/usr/local/bin', - '/usr/local/sbin', '/snap/bin'): - if os.path.exists(os.path.join(directory, cmd)): - return True - - return False + return resolve_command(cmd) is not None def sourced_result(lines: list, source: str, source_label: str) -> dict: diff --git a/backend/tests/test_firewall_detection.py b/backend/tests/test_firewall_detection.py new file mode 100644 index 00000000..5f7b750c --- /dev/null +++ b/backend/tests/test_firewall_detection.py @@ -0,0 +1,200 @@ +"""Firewall detection: an installed firewall must not report as absent. + +Measured on a live panel: `ufw` was installed and `dpkg -l` said `ii`, the +install notification said "UFW installed successfully", and /security/firewall +still rendered "No Firewall Installed". fail2ban worked perfectly on the same +box, which is the clue — `fail2ban-client` lives in /usr/bin, `ufw` in +/usr/sbin, and the panel's systemd unit ships: + + Environment="PATH=/opt/serverkit/venv/bin:/usr/local/bin:/usr/bin:/bin" + +So subprocess could not exec `ufw` by bare name and raised FileNotFoundError. +The service had already computed installed=True correctly; a single try/except +around the whole body then threw that away and returned installed=False. + +Two independent defects, two independent guards: + 1. a command outside $PATH must still be runnable (resolve_command) + 2. a failed status probe must never unsay "installed" +""" + +import subprocess +from unittest.mock import patch + +import pytest + +from app.services.firewall_service import FirewallService +from app.utils.system import is_command_available, privileged_cmd, resolve_command + + +# --------------------------------------------------------------------------- # +# 1. Commands outside $PATH +# --------------------------------------------------------------------------- # + +class TestResolveCommand: + + @patch('app.utils.system.shutil.which', return_value='/usr/bin/nginx') + def test_path_hit_is_used_as_is(self, _which): + assert resolve_command('nginx') == '/usr/bin/nginx' + + @patch('app.utils.system.os.path.exists') + @patch('app.utils.system.shutil.which', return_value=None) + def test_sbin_is_searched_when_path_misses(self, _which, exists): + """The exact live failure: ufw in /usr/sbin, not on the unit's PATH.""" + exists.side_effect = lambda p: p == '/usr/sbin/ufw' + + assert resolve_command('ufw') == '/usr/sbin/ufw' + assert is_command_available('ufw') is True + + @patch('app.utils.system.os.path.exists', return_value=False) + @patch('app.utils.system.shutil.which', return_value=None) + def test_genuinely_absent_command_resolves_to_none(self, _which, _exists): + assert resolve_command('nonexistent') is None + assert is_command_available('nonexistent') is False + + @patch('app.utils.system.os.path.exists', return_value=False) + def test_absolute_path_that_does_not_exist_is_not_invented(self, _exists): + assert resolve_command('/opt/nope/tool') is None + + +class TestPrivilegedCmdResolution: + + @patch('app.utils.system.os.name', 'posix') + @patch('app.utils.system.os.geteuid', return_value=0, create=True) + @patch('app.utils.system.os.path.exists') + @patch('app.utils.system.shutil.which', return_value=None) + def test_unresolvable_command_is_given_an_absolute_path(self, _which, exists, + _euid): + """Without this, subprocess raises FileNotFoundError on argv[0].""" + exists.side_effect = lambda p: p == '/usr/sbin/ufw' + + assert privileged_cmd(['ufw', 'status']) == ['/usr/sbin/ufw', 'status'] + + @patch('app.utils.system.os.name', 'posix') + @patch('app.utils.system.os.geteuid', return_value=0, create=True) + @patch('app.utils.system.shutil.which', return_value='/usr/bin/systemctl') + def test_resolvable_command_is_passed_through_untouched(self, _which, _euid): + """A working PATH must not have its argv rewritten underneath callers.""" + assert privileged_cmd(['systemctl', 'restart', 'nginx']) == [ + 'systemctl', 'restart', 'nginx' + ] + + @patch('app.utils.system.os.name', 'posix') + @patch('app.utils.system.os.geteuid', return_value=0, create=True) + @patch('app.utils.system.os.path.exists', return_value=False) + @patch('app.utils.system.shutil.which', return_value=None) + def test_unfindable_command_is_left_alone(self, _which, _exists, _euid): + """Nothing to resolve to — do not fabricate a path.""" + assert privileged_cmd(['ghost', '--x']) == ['ghost', '--x'] + + @patch('app.utils.system.os.name', 'posix') + @patch('app.utils.system.os.geteuid', return_value=1000, create=True) + @patch('app.utils.system.os.path.exists') + @patch('app.utils.system.shutil.which') + def test_resolution_composes_with_sudo(self, which, exists, _euid): + # sudo itself must stay resolvable, or _needs_sudo() short-circuits and + # the test would pass for the wrong reason. + which.side_effect = lambda c: '/usr/bin/sudo' if c == 'sudo' else None + exists.side_effect = lambda p: p == '/usr/sbin/ufw' + + assert privileged_cmd(['ufw', 'status']) == [ + 'sudo', '-n', '/usr/sbin/ufw', 'status' + ] + + +# --------------------------------------------------------------------------- # +# 2. A failed probe must not unsay "installed" +# --------------------------------------------------------------------------- # + +class TestProbeFailureIsolation: + + @patch('app.services.firewall_service.run_privileged') + @patch('app.services.firewall_service.is_command_available', return_value=True) + @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True) + def test_status_probe_raising_does_not_hide_the_install(self, _pkg, _avail, run): + """The live bug, reproduced exactly.""" + run.side_effect = FileNotFoundError(2, 'No such file or directory', 'ufw') + + ufw = FirewallService._check_ufw() + + assert ufw['installed'] is True # was False — the whole bug + assert ufw['active'] is False # genuinely unknown, reported as off + + @patch('app.services.firewall_service.run_privileged') + @patch('app.services.firewall_service.is_command_available') + @patch('app.services.firewall_service.PackageManager.is_installed') + def test_page_would_render_the_firewall_not_the_empty_state(self, pkg, avail, run): + """`any_installed` is what FirewallTab.jsx gates the empty state on. + + Only ufw is present here — the live box has no firewalld, and saying + "everything is installed" would let the assertion pass for free. + """ + pkg.side_effect = lambda name: name == 'ufw' + avail.side_effect = lambda name: name == 'ufw' + run.side_effect = FileNotFoundError(2, 'No such file or directory', 'ufw') + + status = FirewallService.get_status() + + assert status['any_installed'] is True + assert status['active_firewall'] == 'ufw' + assert status['firewalld']['installed'] is False + + @patch('app.services.firewall_service.run_privileged') + @patch('app.services.firewall_service.is_command_available', return_value=True) + @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True) + def test_installed_but_inactive_is_reported_faithfully(self, _pkg, _avail, run): + """builditdesign's real state: installed, `Status: inactive`.""" + run.return_value = subprocess.CompletedProcess([], 0, stdout='Status: inactive\n', + stderr='') + + status = FirewallService.get_status() + + assert status['any_installed'] is True + assert status['any_active'] is False + assert status['ufw'] == {'installed': True, 'active': False} + + @patch('app.services.firewall_service.run_privileged') + @patch('app.services.firewall_service.is_command_available', return_value=True) + @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True) + def test_active_firewall_is_detected(self, _pkg, _avail, run): + run.return_value = subprocess.CompletedProcess([], 0, stdout='Status: active\n', + stderr='') + + status = FirewallService.get_status() + + assert status['any_active'] is True + assert status['ufw']['active'] is True + + @patch('app.services.firewall_service.is_command_available', return_value=False) + @patch('app.services.firewall_service.PackageManager.is_installed', return_value=False) + def test_genuinely_absent_firewall_still_reports_absent(self, _pkg, _avail): + """The guard must not make everything look installed.""" + status = FirewallService.get_status() + + assert status['any_installed'] is False + assert status['active_firewall'] is None + + @patch('app.services.firewall_service.run_privileged') + @patch('app.services.firewall_service.is_command_available', return_value=True) + @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True) + def test_firewalld_probe_failure_is_isolated_too(self, _pkg, _avail, run): + run.side_effect = FileNotFoundError(2, 'No such file or directory', 'firewall-cmd') + + firewalld = FirewallService._check_firewalld() + + assert firewalld['installed'] is True + assert firewalld['running'] is False + assert firewalld['default_zone'] is None + + @pytest.mark.parametrize('failure', [ + subprocess.TimeoutExpired(cmd=['ufw', 'status'], timeout=30), + PermissionError(13, 'Permission denied'), + OSError(8, 'Exec format error'), + ]) + @patch('app.services.firewall_service.run_privileged') + @patch('app.services.firewall_service.is_command_available', return_value=True) + @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True) + def test_any_probe_failure_keeps_installed_true(self, _pkg, _avail, run, failure): + """A sudo timeout or permission denial is not evidence of absence.""" + run.side_effect = failure + + assert FirewallService._check_ufw()['installed'] is True diff --git a/templates/serverkit-backend.service.in b/templates/serverkit-backend.service.in index 20bf6475..d8375911 100644 --- a/templates/serverkit-backend.service.in +++ b/templates/serverkit-backend.service.in @@ -8,7 +8,13 @@ Type=simple User=@USER@ Group=@USER@ WorkingDirectory=@SERVERKIT_DIR@/backend -Environment="PATH=@SERVERKIT_VENV_DIR@/bin:/usr/local/bin:/usr/bin:/bin" +# sbin dirs are mandatory, not cosmetic: the panel shells out to ufw, iptables +# and nft, all of which live in /usr/sbin. Without them a bare-name exec raises +# FileNotFoundError, which surfaced as "No Firewall Installed" on a host where +# ufw was installed and working. app/utils/system.py resolves such commands to +# an absolute path as a backstop; this keeps anything not routed through it +# working too. +Environment="PATH=@SERVERKIT_VENV_DIR@/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" Environment="FLASK_ENV=production" Environment="SERVERKIT_INSTALL_DIR=@SERVERKIT_DIR@" EnvironmentFile=@SERVERKIT_DIR@/.env