From 242bc4e2e20c6944164fe2495598982b1cfcc4fe Mon Sep 17 00:00:00 2001

From: Eric Garver <e@erig.me>

Date: Tue, 16 Oct 2018 10:02:38 -0400

Subject: [PATCH] Add "FlushAllOnReload" config option

Default value is "yes".

Even on --reload some runtime configuration was being retained. This was

a surprise to many users. Lets default to flushing all the runtime

configuration and also introduce a config option to go back the old

behavior of retaining; interface to zone assignments, and direct rules.

This also adjusts a few test cases that depend on the old

FlushAllOnReload=no behavior.

Fixes: suze bz #1109153

Fixes: #409

Closes: #408

---

 config/firewalld.conf                  |  8 +++

 doc/xml/firewalld.conf.xml             | 13 ++++

 doc/xml/firewalld.dbus.xml             |  8 +++

 src/firewall/config/__init__.py.in     |  1 +

 src/firewall/core/fw.py                | 99 +++++++++++++++-----------

 src/firewall/core/io/firewalld_conf.py | 11 ++-

 src/firewall/server/config.py          | 20 +++++-

 src/tests/dbus/firewalld.conf.at       |  2 +

 src/tests/python/firewalld_direct.py   |  6 ++

 src/tests/python/firewalld_test.py     |  9 ++-

 src/tests/regression/rhbz1498923.at    |  4 ++

 11 files changed, 133 insertions(+), 48 deletions(-)

diff --git a/config/firewalld.conf b/config/firewalld.conf

index b53c0aa50c53..7cb02561fd4d 100644

--- a/config/firewalld.conf

+++ b/config/firewalld.conf

@@ -62,3 +62,11 @@ AutomaticHelpers=system

 #	- nftables (default)

 #	- iptables (iptables, ip6tables, ebtables and ipset)

 FirewallBackend=nftables

+

+# FlushAllOnReload

+# Flush all runtime rules on a reload. In previous releases some runtime

+# configuration was retained during a reload, namely; interface to zone

+# assignment, and direct rules. This was confusing to users. To get the old

+# behavior set this to "no".

+# Default: yes

+FlushAllOnReload=yes

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

index df4b9521fd71..600919ae822d 100644

--- a/doc/xml/firewalld.conf.xml

+++ b/doc/xml/firewalld.conf.xml

@@ -158,6 +158,19 @@

 	        </listitem>

         </varlistentry>

+        <varlistentry>

+            <term><option>FlushAllOnReload</option></term>

+            <listitem>

+                <para>

+                Flush all runtime rules on a reload. In previous releases some

+                runtime configuration was retained during a reload, namely;

+                interface to zone assignment, and direct rules. This was

+                confusing to users. To get the old behavior set this to "no".

+                Defaults to "yes".

+                </para>

+            </listitem>

+        </varlistentry>

+

     </variablelist>

   </refsect1>

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

index 8352f96cc057..132200f3cb42 100644

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

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

@@ -2592,6 +2592,14 @@

               </para>

             </listitem>

           </varlistentry>

+          <varlistentry id="FirewallD1.config.Properties.FlushAllOnReload">

+            <term>FirewallBackend - s - (rw)</term>

+            <listitem>

+              <para>

+                Flush all runtime rules on a reload. Valid options are; yes, no.

+              </para>

+            </listitem>

+          </varlistentry>

           <varlistentry id="FirewallD1.config.Properties.IPv6_rpfilter">

             <term><parameter>IPv6_rpfilter</parameter> - s - (rw)</term>

             <listitem><para>Indicates whether the reverse path filter test on a packet for IPv6 is enabled. If a reply to the packet would be sent via the same interface that the packet arrived on, the packet will match and be accepted, otherwise dropped.</para></listitem>

diff --git a/src/firewall/config/__init__.py.in b/src/firewall/config/__init__.py.in

index 955be32077e1..2cfbef804778 100644

--- a/src/firewall/config/__init__.py.in

+++ b/src/firewall/config/__init__.py.in

@@ -130,3 +130,4 @@ FALLBACK_INDIVIDUAL_CALLS = False

 FALLBACK_LOG_DENIED = "off"

 FALLBACK_AUTOMATIC_HELPERS = "system"

 FALLBACK_FIREWALL_BACKEND = "nftables"

+FALLBACK_FLUSH_ALL_ON_RELOAD = True

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

index 9be13a5c1313..7e6255f77398 100644

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

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

@@ -113,6 +113,7 @@ class Firewall(object):

         self._log_denied = config.FALLBACK_LOG_DENIED

         self._automatic_helpers = config.FALLBACK_AUTOMATIC_HELPERS

         self._firewall_backend = config.FALLBACK_FIREWALL_BACKEND

+        self._flush_all_on_reload = config.FALLBACK_FLUSH_ALL_ON_RELOAD

         self.nf_conntrack_helper_setting = 0

         self.nf_conntrack_helpers = { }

         self.nf_nat_helpers = { }

@@ -298,6 +299,15 @@ class Firewall(object):

                 log.debug1("FirewallBackend is set to '%s'",

                            self._firewall_backend)

+            if self._firewalld_conf.get("FlushAllOnReload"):

+                value = self._firewalld_conf.get("FlushAllOnReload")

+                if value.lower() in [ "no", "false" ]:

+                    self._flush_all_on_reload = False

+                else:

+                    self._flush_all_on_reload = True

+                log.debug1("FlushAllOnReload is set to '%s'",

+                           self._flush_all_on_reload)

+

         self.config.set_firewalld_conf(copy.deepcopy(self._firewalld_conf))

         self._select_firewall_backend(self._firewall_backend)

@@ -963,13 +973,17 @@ class Firewall(object):

     def reload(self, stop=False):

         _panic = self._panic

-        # save zone interfaces

-        _zone_interfaces = { }

-        for zone in self.zone.get_zones():

-            _zone_interfaces[zone] = self.zone.get_settings(zone)["interfaces"]

-        # save direct config

-        _direct_config = self.direct.get_runtime_config()

-        _old_dz = self.get_default_zone()

+        # must stash this. The value may change after _start()

+        flush_all = self._flush_all_on_reload

+

+        if not flush_all:

+            # save zone interfaces

+            _zone_interfaces = { }

+            for zone in self.zone.get_zones():

+                _zone_interfaces[zone] = self.zone.get_settings(zone)["interfaces"]

+            # save direct config

+            _direct_config = self.direct.get_runtime_config()

+            _old_dz = self.get_default_zone()

         # stop

         self.cleanup()

@@ -984,41 +998,42 @@ class Firewall(object):

             # etc. We'll re-raise it at the end.

             start_exception = e

-        # handle interfaces in the default zone and move them to the new

-        # default zone if it changed

-        _new_dz = self.get_default_zone()

-        if _new_dz != _old_dz:

-            # if_new_dz has been introduced with the reload, we need to add it

-            # https://github.com/firewalld/firewalld/issues/53

-            if _new_dz not in _zone_interfaces:

-                _zone_interfaces[_new_dz] = { }

-            # default zone changed. Move interfaces from old default zone to

-            # the new one.

-            for iface, settings in list(_zone_interfaces[_old_dz].items()):

-                if settings["__default__"]:

-                    # move only those that were added to default zone

-                    # (not those that were added to specific zone same as

-                    # default)

-                    _zone_interfaces[_new_dz][iface] = \

-                        _zone_interfaces[_old_dz][iface]

-                    del _zone_interfaces[_old_dz][iface]

-

-        # add interfaces to zones again

-        for zone in self.zone.get_zones():

-            if zone in _zone_interfaces:

-                self.zone.set_settings(zone, { "interfaces":

-                                               _zone_interfaces[zone] })

-                del _zone_interfaces[zone]

-            else:

-                log.info1("New zone '%s'.", zone)

-        if len(_zone_interfaces) > 0:

-            for zone in list(_zone_interfaces.keys()):

-                log.info1("Lost zone '%s', zone interfaces dropped.", zone)

-                del _zone_interfaces[zone]

-        del _zone_interfaces

-

-        # restore direct config

-        self.direct.set_config(_direct_config)

+        if not flush_all:

+            # handle interfaces in the default zone and move them to the new

+            # default zone if it changed

+            _new_dz = self.get_default_zone()

+            if _new_dz != _old_dz:

+                # if_new_dz has been introduced with the reload, we need to add it

+                # https://github.com/firewalld/firewalld/issues/53

+                if _new_dz not in _zone_interfaces:

+                    _zone_interfaces[_new_dz] = { }

+                # default zone changed. Move interfaces from old default zone to

+                # the new one.

+                for iface, settings in list(_zone_interfaces[_old_dz].items()):

+                    if settings["__default__"]:

+                        # move only those that were added to default zone

+                        # (not those that were added to specific zone same as

+                        # default)

+                        _zone_interfaces[_new_dz][iface] = \

+                            _zone_interfaces[_old_dz][iface]

+                        del _zone_interfaces[_old_dz][iface]

+

+            # add interfaces to zones again

+            for zone in self.zone.get_zones():

+                if zone in _zone_interfaces:

+                    self.zone.set_settings(zone, { "interfaces":

+                                                   _zone_interfaces[zone] })

+                    del _zone_interfaces[zone]

+                else:

+                    log.info1("New zone '%s'.", zone)

+            if len(_zone_interfaces) > 0:

+                for zone in list(_zone_interfaces.keys()):

+                    log.info1("Lost zone '%s', zone interfaces dropped.", zone)

+                    del _zone_interfaces[zone]

+            del _zone_interfaces

+

+            # restore direct config

+            self.direct.set_config(_direct_config)

         # enable panic mode again if it has been enabled before or set policy

         # to ACCEPT

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

index 4d57bad693c1..953a6d2618ec 100644

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

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

@@ -30,7 +30,7 @@ from firewall.functions import b2u, u2b, PY2

 valid_keys = [ "DefaultZone", "MinimalMark", "CleanupOnExit", "Lockdown", 

                "IPv6_rpfilter", "IndividualCalls", "LogDenied",

-               "AutomaticHelpers", "FirewallBackend" ]

+               "AutomaticHelpers", "FirewallBackend", "FlushAllOnReload" ]

 class firewalld_conf(object):

     def __init__(self, filename):

@@ -80,6 +80,7 @@ class firewalld_conf(object):

             self.set("LogDenied", config.FALLBACK_LOG_DENIED)

             self.set("AutomaticHelpers", config.FALLBACK_AUTOMATIC_HELPERS)

             self.set("FirewallBackend", config.FALLBACK_FIREWALL_BACKEND)

+            self.set("FlushAllOnReload", "yes" if config.FALLBACK_FLUSH_ALL_ON_RELOAD else "no")

             raise

         for line in f:

@@ -183,6 +184,14 @@ class firewalld_conf(object):

                             config.FALLBACK_FIREWALL_BACKEND)

             self.set("FirewallBackend", str(config.FALLBACK_FIREWALL_BACKEND))

+        value = self.get("FlushAllOnReload")

+        if not value or value.lower() not in [ "yes", "true", "no", "false" ]:

+            if value is not None:

+                log.warning("FlushAllOnReload '%s' is not valid, using default "

+                            "value %s", value if value else '',

+                            config.FALLBACK_FLUSH_ALL_ON_RELOAD)

+            self.set("FlushAllOnReload", str(config.FALLBACK_FLUSH_ALL_ON_RELOAD))

+

     # save to self.filename if there are key/value changes

     def write(self):

         if len(self._config) < 1:

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

index dfc562b537eb..ba04107fe4a1 100644

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

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

@@ -106,6 +106,7 @@ class FirewallDConfig(slip.dbus.service.Object):

                                                 "LogDenied": "readwrite",

                                                 "AutomaticHelpers": "readwrite",

                                                 "FirewallBackend": "readwrite",

+                                                "FlushAllOnReload": "readwrite",

                                               })

     @handle_exceptions

@@ -485,7 +486,8 @@ class FirewallDConfig(slip.dbus.service.Object):

     def _get_property(self, prop):

         if prop not in [ "DefaultZone", "MinimalMark", "CleanupOnExit",

                          "Lockdown", "IPv6_rpfilter", "IndividualCalls",

-                         "LogDenied", "AutomaticHelpers", "FirewallBackend" ]:

+                         "LogDenied", "AutomaticHelpers", "FirewallBackend",

+                         "FlushAllOnReload" ]:

             raise dbus.exceptions.DBusException(

                 "org.freedesktop.DBus.Error.InvalidArgs: "

                 "Property '%s' does not exist" % prop)

@@ -530,6 +532,10 @@ class FirewallDConfig(slip.dbus.service.Object):

             if value is None:

                 value = config.FALLBACK_FIREWALL_BACKEND

             return dbus.String(value)

+        elif prop == "FlushAllOnReload":

+            if value is None:

+                value = "yes" if config.FALLBACK_FLUSH_ALL_ON_RELOAD else "no"

+            return dbus.String(value)

     @dbus_handle_exceptions

     def _get_dbus_property(self, prop):

@@ -551,6 +557,8 @@ class FirewallDConfig(slip.dbus.service.Object):

             return dbus.String(self._get_property(prop))

         elif prop == "FirewallBackend":

             return dbus.String(self._get_property(prop))

+        elif prop == "FlushAllOnReload":

+            return dbus.String(self._get_property(prop))

         else:

             raise dbus.exceptions.DBusException(

                 "org.freedesktop.DBus.Error.InvalidArgs: "

@@ -590,7 +598,8 @@ class FirewallDConfig(slip.dbus.service.Object):

         if interface_name == config.dbus.DBUS_INTERFACE_CONFIG:

             for x in [ "DefaultZone", "MinimalMark", "CleanupOnExit",

                        "Lockdown", "IPv6_rpfilter", "IndividualCalls",

-                       "LogDenied", "AutomaticHelpers", "FirewallBackend" ]:

+                       "LogDenied", "AutomaticHelpers", "FirewallBackend",

+                       "FlushAllOnReload" ]:

                 ret[x] = self._get_property(x)

         elif interface_name in [ config.dbus.DBUS_INTERFACE_CONFIG_DIRECT,

                                  config.dbus.DBUS_INTERFACE_CONFIG_POLICIES ]:

@@ -617,7 +626,7 @@ class FirewallDConfig(slip.dbus.service.Object):

             if property_name in [ "MinimalMark", "CleanupOnExit", "Lockdown",

                                   "IPv6_rpfilter", "IndividualCalls",

                                   "LogDenied", "AutomaticHelpers",

-                                  "FirewallBackend" ]:

+                                  "FirewallBackend", "FlushAllOnReload" ]:

                 if property_name == "MinimalMark":

                     try:

                         int(new_value)

@@ -651,6 +660,11 @@ class FirewallDConfig(slip.dbus.service.Object):

                         raise FirewallError(errors.INVALID_VALUE,

                                             "'%s' for %s" % \

                                             (new_value, property_name))

+                if property_name == "FlushAllOnReload":

+                    if new_value.lower() not in ["yes", "true", "no", "false"]:

+                        raise FirewallError(errors.INVALID_VALUE,

+                                            "'%s' for %s" % \

+                                            (new_value, property_name))

                 self.config.get_firewalld_conf().set(property_name, new_value)

                 self.config.get_firewalld_conf().write()

                 self.PropertiesChanged(interface_name,

diff --git a/src/tests/dbus/firewalld.conf.at b/src/tests/dbus/firewalld.conf.at

index 473210de10af..72c61bdc4940 100644

--- a/src/tests/dbus/firewalld.conf.at

+++ b/src/tests/dbus/firewalld.conf.at

@@ -6,6 +6,7 @@ string "AutomaticHelpers" : variant string "system"

 string "CleanupOnExit" : variant string "no"

 string "DefaultZone" : variant string "public"

 string "FirewallBackend" : variant string "nftables"

+string "FlushAllOnReload" : variant string "yes"

 m4_if(no, HOST_SUPPORTS_NFT_FIB, [dnl

 string "IPv6_rpfilter" : variant string "no"],[dnl

 string "IPv6_rpfilter" : variant string "yes"])

@@ -30,6 +31,7 @@ _helper([LogDenied], [string:"all"], [variant string "all"])

 _helper([IPv6_rpfilter], [string:"yes"], [variant string "yes"])

 _helper([IndividualCalls], [string:"yes"], [variant string "yes"])

 _helper([FirewallBackend], [string:"iptables"], [variant string "iptables"])

+_helper([FlushAllOnReload], [string:"no"], [variant string "no"])

 _helper([CleanupOnExit], [string:"yes"], [variant string "yes"])

 dnl Note: DefaultZone is RO

 m4_undefine([_helper])

diff --git a/src/tests/python/firewalld_direct.py b/src/tests/python/firewalld_direct.py

index 4cb843492663..28da523d35d0 100755

--- a/src/tests/python/firewalld_direct.py

+++ b/src/tests/python/firewalld_direct.py

@@ -36,10 +36,16 @@ class TestFirewallDInterfaceDirect(unittest.TestCase):

         bus = dbus.SystemBus()

         dbus_obj = bus.get_object(config.dbus.DBUS_INTERFACE,

                                   config.dbus.DBUS_PATH)

+        dbus_obj_config = bus.get_object(config.dbus.DBUS_INTERFACE,

+                                         config.dbus.DBUS_PATH_CONFIG)

         self.fw = dbus.Interface(dbus_obj,

                                  dbus_interface=config.dbus.DBUS_INTERFACE)

         self.fw_direct = dbus.Interface(

             dbus_obj, dbus_interface=config.dbus.DBUS_INTERFACE_DIRECT)

+        self.config_properties = dbus.Interface(dbus_obj_config,

+                                    dbus_interface='org.freedesktop.DBus.Properties')

+        self.config_properties.Set(config.dbus.DBUS_INTERFACE_CONFIG, "FlushAllOnReload", "no")

+        self.fw.reload()

         # always have "direct_foo1" available

         self.fw_direct.addChain("ipv4", "filter", "direct_foo1")

diff --git a/src/tests/python/firewalld_test.py b/src/tests/python/firewalld_test.py

index 62c567fcd299..0d8b4c78bd51 100755

--- a/src/tests/python/firewalld_test.py

+++ b/src/tests/python/firewalld_test.py

@@ -28,8 +28,8 @@ import sys

 import time

 import unittest

-from firewall.config.dbus import DBUS_PATH, DBUS_INTERFACE, \

-                                 DBUS_INTERFACE_ZONE

+from firewall.config.dbus import DBUS_PATH, DBUS_PATH_CONFIG, DBUS_INTERFACE, \

+                                 DBUS_INTERFACE_ZONE, DBUS_INTERFACE_CONFIG

 from firewall.dbus_utils import dbus_to_python

 from pprint import pprint

@@ -43,9 +43,14 @@ class TestFirewallD(unittest.TestCase):

         unittest.TestCase.setUp(self)

         bus = dbus.SystemBus()

         dbus_obj = bus.get_object(DBUS_INTERFACE, DBUS_PATH)

+        dbus_obj_config = bus.get_object(DBUS_INTERFACE, DBUS_PATH_CONFIG)

         self.fw = dbus.Interface(dbus_obj, dbus_interface=DBUS_INTERFACE)

         self.fw_zone = dbus.Interface(dbus_obj,

                                      dbus_interface=DBUS_INTERFACE_ZONE)

+        self.config_properties = dbus.Interface(dbus_obj_config,

+                                    dbus_interface='org.freedesktop.DBus.Properties')

+        self.config_properties.Set(DBUS_INTERFACE_CONFIG, "FlushAllOnReload", "no")

+        self.fw.reload()

     def test_get_setDefaultZone(self):

         old_zone = dbus_to_python(self.fw.getDefaultZone())

diff --git a/src/tests/regression/rhbz1498923.at b/src/tests/regression/rhbz1498923.at

index 9b68678180ef..ed1022fb0ca4 100644

--- a/src/tests/regression/rhbz1498923.at

+++ b/src/tests/regression/rhbz1498923.at

@@ -1,4 +1,8 @@

 FWD_START_TEST([invalid direct rule causes reload error])

+dnl Below we test retention of some items applicable to FlushAllOnReload=no

+AT_CHECK([sed -i 's/^FlushAllOnReload.*/FlushAllOnReload=no/' ./firewalld.conf])

+FWD_RELOAD

+

 FWD_CHECK([-q --permanent --direct --add-rule ipv4 filter INPUT 0 -p tcp --dport 8080 -j ACCEPT])

 FWD_CHECK([-q --permanent --direct --add-rule ipv4 filter INPUT 1 --a-bogus-flag])

-- 

2.18.0