costales / gufw

Linux Firewall
GNU General Public License v3.0
129 stars 33 forks source link

Status toggle button wipes out gufw rules from the profile, and reloads them from ufw #54

Closed dominikborkowski closed 1 year ago

dominikborkowski commented 2 years ago

This is a baffling behavior: turning the firewall off with the toggle button next to Status wipes out all gufw rules from its profile file. Toggling it back on reads them from UFW.

This wouldn't be a problem if not for the fact that Gufw cannot modify rules that originate from UFW, only remove them. So by a mere act of disabling/renabling firewall, user is left with immutable rules. Unless I'm missing something obvious, this is hardly a desirable behavior.

This was tested on latest Ubuntu 22. I believe there is Ubuntu bug that was filed over a year ago, but there seems to be no traction: https://bugs.launchpad.net/gui-ufw/+bug/1956171

dominikborkowski commented 2 years ago

Found the culprit of this behavior, and proposed a fix via this PR: https://github.com/costales/gufw/pull/55

deadwood2 commented 1 year ago

Here is my take at fixing this bug. As far as I can see this is related to wrong state management. Writing profile at status change requires reading rules. Reading ufw rules requires issuing /usr/sbin/ufw status numbered. This will only return rules if ufw is enabled. However in current code, we first disable ufw and only then try to read the rules which results with empty list of rules and no matching to profiles rules. Then we save empty list, wiping out all profile rules.

--- gufw/gufw/model/firewall.py 2023-04-04 17:54:30.882633595 +0200
+++ firewall.py 2023-04-04 17:58:03.000000000 +0200
@@ -131,8 +131,18 @@

     def set_status(self, status):
         self.status = status
+
+        if status == True:
+            self.backend.set_status(status)
+
+        # get_rules() need to be called on enabled firewall, otherwise they will
+        # wipe out profile rules, because the colleciton of ufw_rules will be
+        # empty
         self.backend.set_profile_values(self.profile, status, self.incoming, self.outgoing, self.routed, self.get_rules(False))
-        self.backend.set_status(status)
+
+        if status == False:
+            self.backend.set_status(status)
+

     def get_policy(self, policy):
         if policy == 'incoming':
costales commented 1 year ago

Fixed in next release. Thank you for the PR!!