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 <veiko17@disroot.org>

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

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)
+    zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE)
-    return output.split()
+    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],
+        config = _get_dbus_proxy(_CONFIG_OBJECT, _CONFIG_INTERFACE)
-                      superuser=True)
+        try:
-        _port_details[service_port] = output.strip()
+            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)
+    zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE)
-    return output.split()
+    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):

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

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

plinth/modules/firewall/templates/firewall.html

@@ -71,7 +71,11 @@
               <tr class="collapse out {{component.component_id}}"
                   style="background-color: #f9f9f9" >
                 <td></td>
-                <td class='cell-indented'><em>{{ port.name }}</em> ({{ port.details }})</td>
+                <td class='cell-indented'><em>{{ port.name }}</em>:
+                  {% for port_number, protocol in port.details %}
+                    {{ port_number }}/{{ protocol }}
+                  {% endfor %}
+                </td>
                 <td>
                   {% if port.name in internal_enabled_ports and port.name in external_enabled_ports %}
                     <span class='label label-success'>

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-port1': [(1234, 'tcp'), (1234, 'udp')],
-            'test-port2': '2345/udp',
+            'test-port2': [(2345, 'udp')],
-            'test-port3': '3456/tcp',
+            'test-port3': [(3456, 'tcp')],
-            'test-port4': '4567/udp'
+            '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',
+            'Port test-port1 (1234/tcp, 1234/udp) available for internal '
-            'passed'
+            'networks', 'passed'
         ],
         [
-            'Port test-port1 (1234/tcp) unavailable for external networks',
+            'Port test-port1 (1234/tcp, 1234/udp) unavailable for external '
-            'passed'
+            'networks', 'passed'
         ],
         [
             'Port test-port2 (2345/udp) available for internal networks',