From 905f7eb62dd31a58b86fbfa191b2ce2482361b0b Mon Sep 17 00:00:00 2001

From: Eric Garver <eric@garver.life>

Date: Mon, 24 Jun 2019 10:36:40 -0400

Subject: [PATCH 10/20] fix: dbus: fix service API break

This fixes a dbus API break that occurred when introducing service

includes. The includes were added to the method's tuple, but doing so

changed the dbus signature and thus broke the API. This restores the old

signature.

Move to using key,value based import/export and sanity checking.

Previously we were using a tuple with semi-undocumented positions.

Fixes: 1fc208bf9317 ("feat: service includes")

Fixes: rhbz 1721414

(cherry picked from commit 335a68c1bba5b1b1fbd430505a485a9eb035360c)

---

 doc/xml/firewalld.dbus.xml       |  5 ++-

 src/firewall/core/fw_config.py   | 59 +++++++++++++++++++++++++++++++-

 src/firewall/core/io/service.py  | 35 ++++++++++++++++---

 src/firewall/server/firewalld.py | 13 ++++++-

 4 files changed, 103 insertions(+), 9 deletions(-)

diff --git a/doc/xml/firewalld.dbus.xml b/doc/xml/firewalld.dbus.xml

index 64d4d2b9c73b..cb4e1eac0fb9 100644

--- a/doc/xml/firewalld.dbus.xml

+++ b/doc/xml/firewalld.dbus.xml

@@ -242,12 +242,12 @@

             </listitem>

           </varlistentry>

 	  <varlistentry id="FirewallD1.Methods.getServiceSettings">

-            <term><methodname>getServiceSettings</methodname>(s: <parameter>service</parameter>) → (sssa(ss)asa{ss}asa(ss)as)</term>

+            <term><methodname>getServiceSettings</methodname>(s: <parameter>service</parameter>) → (sssa(ss)asa{ss}asa(ss))</term>

             <listitem>

               <para>

                 Return runtime settings of given <replaceable>service</replaceable>.

                 For getting permanent settings see <link linkend="FirewallD1.config.service.Methods.getSettings">org.fedoraproject.FirewallD1.config.service.Methods.getSettings</link>.

-                Settings are in format: <parameter>version</parameter>, <parameter>name</parameter>, <parameter>description</parameter>, array of <parameter>ports</parameter> (port, protocol), array of <parameter>module names</parameter>, dictionary of <parameter>destinations</parameter>, array of <parameter>protocols</parameter>, array of <parameter>source-ports</parameter> (port, protocol) and array of service <parameter>includes</parameter>.

+                Settings are in format: <parameter>version</parameter>, <parameter>name</parameter>, <parameter>description</parameter>, array of <parameter>ports</parameter> (port, protocol), array of <parameter>module names</parameter>, dictionary of <parameter>destinations</parameter>, array of <parameter>protocols</parameter>, array of <parameter>source-ports</parameter> (port, protocol).

               </para>

               <para>

                 <variablelist>

@@ -259,7 +259,6 @@

                   <varlistentry><term><parameter>destinations (a{ss})</parameter>: dictionary of {IP family : IP address} where 'IP family' key can be either 'ipv4' or 'ipv6'. See <literal>destination</literal> tag in <citerefentry><refentrytitle>firewalld.service</refentrytitle><manvolnum>5</manvolnum></citerefentry>.</term></varlistentry>

                   <varlistentry><term><parameter>protocols (as)</parameter>: array of protocols, see <literal>protocol</literal> tag in <citerefentry><refentrytitle>firewalld.service</refentrytitle><manvolnum>5</manvolnum></citerefentry>.</term></varlistentry>

 		  <varlistentry><term><parameter>source-ports (a(ss))</parameter>: array of port and protocol pairs. See <literal>source-port</literal> tag in <citerefentry><refentrytitle>firewalld.service</refentrytitle><manvolnum>5</manvolnum></citerefentry>.</term></varlistentry>

-                  <varlistentry><term><parameter>includes (as)</parameter>: array of service includes, see <literal>include</literal> tag in <citerefentry><refentrytitle>firewalld.service</refentrytitle><manvolnum>5</manvolnum></citerefentry>.</term></varlistentry>

                 </variablelist>

               </para>

 	      <para>

diff --git a/src/firewall/core/fw_config.py b/src/firewall/core/fw_config.py

index a759cfdf83b3..8f29f0c416d2 100644

--- a/src/firewall/core/fw_config.py

+++ b/src/firewall/core/fw_config.py

@@ -545,9 +545,43 @@ class FirewallConfig(object):

         return self._builtin_services[obj.name]

     def get_service_config(self, obj):

+        conf_dict = obj.export_config()

+        conf_list = []

+        for i in range(8): # tuple based dbus API has 8 elements

+            if obj.IMPORT_EXPORT_STRUCTURE[i][0] not in conf_dict:

+                # old API needs the empty elements as well. Grab it from the

+                # object otherwise we don't know the type.

+                conf_list.append(copy.deepcopy(getattr(obj, obj.IMPORT_EXPORT_STRUCTURE[i][0])))

+            else:

+                conf_list.append(conf_dict[obj.IMPORT_EXPORT_STRUCTURE[i][0]])

+        return tuple(conf_list)

+

+    def get_service_config_dict(self, obj):

         return obj.export_config()

     def set_service_config(self, obj, conf):

+        conf_dict = {}

+        for i,value in enumerate(conf):

+            conf_dict[obj.IMPORT_EXPORT_STRUCTURE[i][0]] = value

+

+        if obj.builtin:

+            x = copy.copy(obj)

+            x.cleanup()

+            x.import_config(conf_dict)

+            x.path = config.ETC_FIREWALLD_SERVICES

+            x.builtin = False

+            if obj.path != x.path:

+                x.default = False

+            self.add_service(x)

+            service_writer(x)

+            return x

+        else:

+            obj.cleanup()

+            obj.import_config(conf_dict)

+            service_writer(obj)

+            return obj

+

+    def set_service_config_dict(self, obj, conf):

         if obj.builtin:

             x = copy.copy(obj)

             x.import_config(conf)

@@ -568,6 +602,29 @@ class FirewallConfig(object):

             raise FirewallError(errors.NAME_CONFLICT,

                                 "new_service(): '%s'" % name)

+        conf_dict = {}

+        for i,value in enumerate(conf):

+            conf_dict[Service.IMPORT_EXPORT_STRUCTURE[i][0]] = value

+

+        x = Service()

+        x.check_name(name)

+        x.import_config(conf_dict)

+        x.name = name

+        x.filename = "%s.xml" % name

+        x.path = config.ETC_FIREWALLD_SERVICES

+        # It is not possible to add a new one with a name of a buitin

+        x.builtin = False

+        x.default = True

+

+        service_writer(x)

+        self.add_service(x)

+        return x

+

+    def new_service_dict(self, name, conf):

+        if name in self._services or name in self._builtin_services:

+            raise FirewallError(errors.NAME_CONFLICT,

+                                "new_service(): '%s'" % name)

+

         x = Service()

         x.check_name(name)

         x.import_config(conf)

@@ -684,7 +741,7 @@ class FirewallConfig(object):

         return new_service

     def _copy_service(self, obj, name):

-        return self.new_service(name, obj.export_config())

+        return self.new_service_dict(name, obj.export_config())

     # zones

diff --git a/src/firewall/core/io/service.py b/src/firewall/core/io/service.py

index 3479dab7f175..44dc0ff8a9b0 100644

--- a/src/firewall/core/io/service.py

+++ b/src/firewall/core/io/service.py

@@ -25,6 +25,8 @@ import xml.sax as sax

 import os

 import io

 import shutil

+import copy

+from collections import OrderedDict

 from firewall import config

 from firewall.functions import u2b_if_py2

@@ -47,7 +49,7 @@ class Service(IO_Object):

         ( "source_ports", [ ( "", "" ), ], ), # a(ss)

         ( "includes", [ "" ], ),              # as

         )

-    DBUS_SIGNATURE = '(sssa(ss)asa{ss}asa(ss)as)'

+    DBUS_SIGNATURE = '(sssa(ss)asa{ss}asa(ss))'

     ADDITIONAL_ALNUM_CHARS = [ "_", "-" ]

     PARSER_REQUIRED_ELEMENT_ATTRS = {

         "short": None,

@@ -76,6 +78,34 @@ class Service(IO_Object):

         self.source_ports = [ ]

         self.includes = [ ]

+    def import_config(self, conf):

+        self.check_config(conf)

+

+        for key in conf:

+            if not hasattr(self, key):

+                raise FirewallError(errors.UNKNOWN_ERROR, "Internal error. '{}' is not a valid attribute".format(key))

+            if isinstance(conf[key], list):

+                # maintain list order while removing duplicates

+                setattr(self, key, list(OrderedDict.fromkeys(copy.deepcopy(conf[key]))))

+            else:

+                setattr(self, key, copy.deepcopy(conf[key]))

+

+    def export_config(self):

+        conf = {}

+        type_formats = dict([(x[0], x[1]) for x in self.IMPORT_EXPORT_STRUCTURE])

+        for key in type_formats:

+            if getattr(self, key):

+                conf[key] = copy.deepcopy(getattr(self, key))

+        return conf

+

+    def check_config(self, conf):

+        type_formats = dict([(x[0], x[1]) for x in self.IMPORT_EXPORT_STRUCTURE])

+        for key in conf:

+            if key not in [x for (x,y) in self.IMPORT_EXPORT_STRUCTURE]:

+                raise FirewallError(errors.INVALID_OPTION, "service option '{}' is not valid".format(key))

+            self._check_config_structure(conf[key], type_formats[key])

+            self._check_config(conf[key], key)

+

     def cleanup(self):

         self.version = ""

         self.short = ""

@@ -138,9 +168,6 @@ class Service(IO_Object):

                 if len(module) < 2:

                     raise FirewallError(errors.INVALID_MODULE, module)

-        elif item == "includes":

-            pass

-

 # PARSER

 class service_ContentHandler(IO_Object_ContentHandler):

diff --git a/src/firewall/server/firewalld.py b/src/firewall/server/firewalld.py

index bc04f2d0f4c3..233160b64b18 100644

--- a/src/firewall/server/firewalld.py

+++ b/src/firewall/server/firewalld.py

@@ -26,6 +26,7 @@ from gi.repository import GLib, GObject

 import sys

 sys.modules['gobject'] = GObject

+import copy

 import dbus

 import dbus.service

 import slip.dbus

@@ -921,7 +922,17 @@ class FirewallD(slip.dbus.service.Object):

         # returns service settings for service

         service = dbus_to_python(service, str)

         log.debug1("getServiceSettings(%s)", service)

-        return self.fw.service.get_service(service).export_config()

+        obj = self.fw.service.get_service(service)

+        conf_dict = obj.export_config()

+        conf_list = []

+        for i in range(8): # tuple based dbus API has 8 elements

+            if obj.IMPORT_EXPORT_STRUCTURE[i][0] not in conf_dict:

+                # old API needs the empty elements as well. Grab it from the

+                # object otherwise we don't know the type.

+                conf_list.append(copy.deepcopy(getattr(obj, obj.IMPORT_EXPORT_STRUCTURE[i][0])))

+            else:

+                conf_list.append(conf_dict[obj.IMPORT_EXPORT_STRUCTURE[i][0]])

+        return tuple(conf_list)

     @slip.dbus.polkit.require_auth(config.dbus.PK_ACTION_INFO)

     @dbus_service_method(config.dbus.DBUS_INTERFACE, in_signature='',

-- 

2.20.1