From 92f6f8723c3c68369c1a3acef80fcb7d43e7e1dc Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Fri, 14 Feb 2020 13:44:58 -0800 Subject: [PATCH] firewall: Use firewalld DBus API for most operations - Significantly reduce the time taken to perform the operations in the following cases: - Enable/disable an app - list services in firewall page - Load app page when interfaces belonging to internal zone need to shown - First run setup of FreedomBox - Install a polkit local authority configuration file to allow FreedomBox service to perform: information queries and configuration changes. - Drop unused actions. - Alter the template for showing firewall port information since port/protocol pairs are no longer pre-formatted. - Handle errors when trying to get ports details of an unknown service. Tests performed: - Enable/disable an app. Ensure with firewall-cmd that ports are added/removed properly. - Temporarily modify code to call add_service() and remove_service() twice in a row. Perform enable/disable operations and ensure that there are not error thrown to test that add/remove services operations are idempotent. - Visit the firewalld page and see the current state is reflected properly. - Visit an app that shows the list of interfaces in firewall zone. Internal interfaces should be listed properly. - Reset the installed version of firewall app and disable all firewall services. Start FreedomBox and ensure that when setup is re-run, default ports (http, https, dns and dhcp) are opened properly. Run again but with ports already enabled to check that the setup operation is idempotent. - Visit diagnostics of an app that uses firewall components and see that ports are listed properly in the port diagnostic test result. - Remove some needed services such as those in /etc/firewalld/services and try to visit the firewalld page. The page should show blank details against the affected services. Reviewed-by: Veiko Aasa --- actions/firewall | 88 ------------------- plinth/modules/firewall/__init__.py | 87 +++++++++++++++--- plinth/modules/firewall/components.py | 9 +- .../org.freedombox.FirewallD1.pkla | 4 + .../modules/firewall/templates/firewall.html | 6 +- .../modules/firewall/tests/test_components.py | 25 +++--- 6 files changed, 104 insertions(+), 115 deletions(-) create mode 100644 plinth/modules/firewall/data/var/lib/polkit-1/localauthority/10-vendor.d/org.freedombox.FirewallD1.pkla diff --git a/actions/firewall b/actions/firewall index a7d00b85d..048ba3c60 100755 --- a/actions/firewall +++ b/actions/firewall @@ -39,42 +39,6 @@ def parse_arguments(): subparsers.add_parser('get-status', help='Get whether firewalld is running') - # Get service status - get_enabled_services = subparsers.add_parser( - 'get-enabled-services', help='Get list of enabled services') - get_enabled_services.add_argument( - '--zone', help='Zone from which the list is to be retrieved', - required=True) - - # Get service ports - get_service_ports = subparsers.add_parser( - 'get-service-ports', help='Get list of ports for service') - get_service_ports.add_argument('--service', help='Name of service', - required=True) - - # Get interface status - get_interfaces = subparsers.add_parser( - 'get-interfaces', help='Get list of interfaces in a zone') - get_interfaces.add_argument( - '--zone', help='Zone from which the list is to be retrieved', - required=True) - - # Add a service - add_service = subparsers.add_parser('add-service', help='Add a service') - add_service.add_argument('service', help='Name of the service to add') - add_service.add_argument('--zone', - help='Zone to which service is to be added', - required=True) - - # Remove a service status - remove_service = subparsers.add_parser('remove-service', - help='Remove a service') - remove_service.add_argument('service', - help='Name of the service to remove') - remove_service.add_argument( - '--zone', help='Zone from which service is to be removed', - required=True) - subparsers.required = True return parser.parse_args() @@ -138,64 +102,12 @@ def subcommand_setup(_): subprocess.call(['firewall-cmd', '--set-default-zone=external']) set_firewall_backend('nftables') - add_service('external', 'http') - add_service('internal', 'http') - add_service('external', 'https') - add_service('internal', 'https') - add_service('internal', 'dns') - add_service('internal', 'dhcp') - def subcommand_get_status(_): """Print status of the firewalld service""" subprocess.call(['firewall-cmd', '--state']) -def subcommand_get_enabled_services(arguments): - """Print the status of variours services""" - subprocess.call( - ['firewall-cmd', '--zone', arguments.zone, '--list-services']) - - -def subcommand_get_service_ports(arguments): - """Print list of ports for service""" - subprocess.call([ - 'firewall-cmd', '--permanent', '--service', arguments.service, - '--get-ports' - ]) - - -def subcommand_get_interfaces(arguments): - """Print the list of interfaces in a zone.""" - subprocess.call( - ['firewall-cmd', '--zone', arguments.zone, '--list-interfaces']) - - -def subcommand_add_service(arguments): - """Permit a service in the firewall.""" - add_service(arguments.zone, arguments.service) - - -def add_service(zone, service): - """Permit a service in the firewall.""" - subprocess.call(['firewall-cmd', '--zone', zone, '--add-service', service]) - subprocess.call([ - 'firewall-cmd', '--zone', zone, '--permanent', '--add-service', service - ]) - - -def subcommand_remove_service(arguments): - """Block a service in the firewall""" - subprocess.call([ - 'firewall-cmd', '--zone', arguments.zone, '--remove-service', - arguments.service - ]) - subprocess.call([ - 'firewall-cmd', '--zone', arguments.zone, '--permanent', - '--remove-service', arguments.service - ]) - - def main(): """Parse arguments and perform all duties""" arguments = parse_arguments() diff --git a/plinth/modules/firewall/__init__.py b/plinth/modules/firewall/__init__.py index eb3c84007..15d6521cb 100644 --- a/plinth/modules/firewall/__init__.py +++ b/plinth/modules/firewall/__init__.py @@ -18,16 +18,21 @@ FreedomBox app to configure a firewall. """ +import contextlib + from django.utils.translation import ugettext_lazy as _ from plinth import actions from plinth import app as app_module from plinth import cfg, menu from plinth.daemon import Daemon -from plinth.utils import Version, format_lazy +from plinth.utils import Version, format_lazy, import_from_gi from .manifest import backup # noqa, pylint: disable=unused-import +gio = import_from_gi('Gio', '2.0') +glib = import_from_gi('GLib', '2.0') + version = 2 is_essential = True @@ -48,6 +53,15 @@ _port_details = {} app = None +_DBUS_NAME = 'org.fedoraproject.FirewallD1' +_FIREWALLD_OBJECT = '/org/fedoraproject/FirewallD1' +_FIREWALLD_INTERFACE = 'org.fedoraproject.FirewallD1' +_ZONE_INTERFACE = 'org.fedoraproject.FirewallD1.zone' +_CONFIG_OBJECT = '/org/fedoraproject/FirewallD1/config' +_CONFIG_INTERFACE = 'org.fedoraproject.FirewallD1.config' +_CONFIG_SERVICE_INTERFACE = 'org.fedoraproject.FirewallD1.config.service' +_CONFIG_ZONE_INTERFACE = 'org.fedoraproject.FirewallD1.config.zone' + class FirewallApp(app_module.App): """FreedomBox app for Firewall.""" @@ -78,10 +92,21 @@ def init(): app.set_enabled(True) +def _run_setup(): + """Run firewalld setup.""" + _run(['setup'], superuser=True) + add_service('http', 'external') + add_service('http', 'internal') + add_service('https', 'external') + add_service('https', 'internal') + add_service('dns', 'internal') + add_service('dhcp', 'internal') + + def setup(helper, old_version=None): """Install and configure the module.""" helper.install(managed_packages) - _run(['setup'], superuser=True) + _run_setup() def force_upgrade(helper, packages): @@ -96,10 +121,27 @@ def force_upgrade(helper, packages): return False helper.install(['firewalld'], force_configuration='new') - _run(['setup'], superuser=True) + _run_setup() return True +def _get_dbus_proxy(object, interface): + """Return a DBusProxy for a given firewalld object and interface.""" + connection = gio.bus_get_sync(gio.BusType.SYSTEM) + return gio.DBusProxy.new_sync(connection, gio.DBusProxyFlags.NONE, None, + _DBUS_NAME, object, interface) + + +@contextlib.contextmanager +def ignore_dbus_error(ignored_exception): + try: + yield + except glib.Error as exception: + parts = exception.message.split(':') + if parts[0] != 'GDBus.Error' or parts[2].strip() != ignored_exception: + raise + + def get_enabled_status(): """Return whether firewall is enabled""" output = _run(['get-status'], superuser=True) @@ -111,8 +153,8 @@ def get_enabled_status(): def get_enabled_services(zone): """Return the status of various services currently enabled""" - output = _run(['get-enabled-services', '--zone', zone], superuser=True) - return output.split() + zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE) + return zone_proxy.getServices('(s)', zone) def get_port_details(service_port): @@ -120,26 +162,47 @@ def get_port_details(service_port): try: return _port_details[service_port] except KeyError: - output = _run(['get-service-ports', '--service', service_port], - superuser=True) - _port_details[service_port] = output.strip() + config = _get_dbus_proxy(_CONFIG_OBJECT, _CONFIG_INTERFACE) + try: + service_path = config.getServiceByName('(s)', service_port) + except glib.Error: + return [] # Don't cache the error result + + service = _get_dbus_proxy(service_path, _CONFIG_SERVICE_INTERFACE) + _port_details[service_port] = service.getPorts() return _port_details[service_port] def get_interfaces(zone): """Return the list of interfaces in a zone.""" - output = _run(['get-interfaces', '--zone', zone], superuser=True) - return output.split() + zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE) + return zone_proxy.getInterfaces('(s)', zone) def add_service(port, zone): """Enable a service in firewall""" - _run(['add-service', port, '--zone', zone], superuser=True) + zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE) + with ignore_dbus_error('ALREADY_ENABLED'): + zone_proxy.addService('(ssi)', zone, port, 0) + + config = _get_dbus_proxy(_CONFIG_OBJECT, _CONFIG_INTERFACE) + zone_path = config.getZoneByName('(s)', zone) + config_zone = _get_dbus_proxy(zone_path, _CONFIG_ZONE_INTERFACE) + with ignore_dbus_error('ALREADY_ENABLED'): + config_zone.addService('(s)', port) def remove_service(port, zone): """Remove a service in firewall""" - _run(['remove-service', port, '--zone', zone], superuser=True) + zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE) + with ignore_dbus_error('NOT_ENABLED'): + zone_proxy.removeService('(ss)', zone, port) + + config = _get_dbus_proxy(_CONFIG_OBJECT, _CONFIG_INTERFACE) + zone_path = config.getZoneByName('(s)', zone) + config_zone = _get_dbus_proxy(zone_path, _CONFIG_ZONE_INTERFACE) + with ignore_dbus_error('NOT_ENABLED'): + config_zone.removeService('(s)', port) def _run(arguments, superuser=False): diff --git a/plinth/modules/firewall/components.py b/plinth/modules/firewall/components.py index 5af546f73..683a48d2b 100644 --- a/plinth/modules/firewall/components.py +++ b/plinth/modules/firewall/components.py @@ -123,12 +123,15 @@ class Firewall(app.FollowerComponent): external_ports = firewall.get_enabled_services(zone='external') for port_detail in self.ports_details: port = port_detail['name'] + details = ', '.join( + (f'{port_number}/{protocol}' + for port_number, protocol in port_detail['details'])) # Internal zone result = 'passed' if port in internal_ports else 'failed' message = _( 'Port {name} ({details}) available for internal networks' - ).format(name=port, details=port_detail['details']) + ).format(name=port, details=details) results.append([message, result]) # External zone @@ -136,12 +139,12 @@ class Firewall(app.FollowerComponent): result = 'passed' if port in external_ports else 'failed' message = _( 'Port {name} ({details}) available for external networks' - ).format(name=port, details=port_detail['details']) + ).format(name=port, details=details) else: result = 'passed' if port not in external_ports else 'failed' message = _( 'Port {name} ({details}) unavailable for external networks' - ).format(name=port, details=port_detail['details']) + ).format(name=port, details=details) results.append([message, result]) diff --git a/plinth/modules/firewall/data/var/lib/polkit-1/localauthority/10-vendor.d/org.freedombox.FirewallD1.pkla b/plinth/modules/firewall/data/var/lib/polkit-1/localauthority/10-vendor.d/org.freedombox.FirewallD1.pkla new file mode 100644 index 000000000..75759c255 --- /dev/null +++ b/plinth/modules/firewall/data/var/lib/polkit-1/localauthority/10-vendor.d/org.freedombox.FirewallD1.pkla @@ -0,0 +1,4 @@ +[Allow FreedomBox to manage firewalld] +Identity=unix-user:plinth +Action=org.fedoraproject.FirewallD1.config.info;org.fedoraproject.FirewallD1.config +ResultAny=yes diff --git a/plinth/modules/firewall/templates/firewall.html b/plinth/modules/firewall/templates/firewall.html index 584645101..72a8d3e0a 100644 --- a/plinth/modules/firewall/templates/firewall.html +++ b/plinth/modules/firewall/templates/firewall.html @@ -71,7 +71,11 @@ - {{ port.name }} ({{ port.details }}) + {{ port.name }}: + {% for port_number, protocol in port.details %} + {{ port_number }}/{{ protocol }} + {% endfor %} + {% if port.name in internal_enabled_ports and port.name in external_enabled_ports %} diff --git a/plinth/modules/firewall/tests/test_components.py b/plinth/modules/firewall/tests/test_components.py index dc4e90969..1f6118e49 100644 --- a/plinth/modules/firewall/tests/test_components.py +++ b/plinth/modules/firewall/tests/test_components.py @@ -57,7 +57,10 @@ def test_init(): @patch('plinth.modules.firewall.get_port_details') def test_port_details(get_port_details): """Test retrieving port details for a firewall component.""" - return_values = {'test-port1': '1234/tcp', 'test-port2': '5678/udp'} + return_values = { + 'test-port1': [(1234, 'tcp')], + 'test-port2': [(5678, 'udp')] + } def get_port_details_side_effect(port): return return_values[port] @@ -66,10 +69,10 @@ def test_port_details(get_port_details): firewall = Firewall('test-component', ports=['test-port1', 'test-port2']) assert firewall.ports_details == [{ 'name': 'test-port1', - 'details': '1234/tcp' + 'details': [(1234, 'tcp')] }, { 'name': 'test-port2', - 'details': '5678/udp' + 'details': [(5678, 'udp')] }] @@ -144,10 +147,10 @@ def test_diagnose(get_enabled_services, get_port_details): """Test diagnosing open/closed firewall ports.""" def get_port_details_side_effect(port): return { - 'test-port1': '1234/tcp', - 'test-port2': '2345/udp', - 'test-port3': '3456/tcp', - 'test-port4': '4567/udp' + 'test-port1': [(1234, 'tcp'), (1234, 'udp')], + 'test-port2': [(2345, 'udp')], + 'test-port3': [(3456, 'tcp')], + 'test-port4': [(4567, 'udp')] }[port] def get_enabled_services_side_effect(zone): @@ -163,12 +166,12 @@ def test_diagnose(get_enabled_services, get_port_details): results = firewall.diagnose() assert results == [ [ - 'Port test-port1 (1234/tcp) available for internal networks', - 'passed' + 'Port test-port1 (1234/tcp, 1234/udp) available for internal ' + 'networks', 'passed' ], [ - 'Port test-port1 (1234/tcp) unavailable for external networks', - 'passed' + 'Port test-port1 (1234/tcp, 1234/udp) unavailable for external ' + 'networks', 'passed' ], [ 'Port test-port2 (2345/udp) available for internal networks',