[apparmor] [patch] Implement in-profile de-duplication in BaseRuleset

[Christian Boltz]

Hello,

Am Donnerstag, 23. April 2015 schrieb Seth Arnold:
> On Sun, Apr 12, 2015 at 03:29:44AM +0200, Christian Boltz wrote:
> > this patch implements in-profile de-duplication in BaseRuleset
> > (currently affects "only" CapabilityRuleset, but will also work for
> > all future *Ruleset classes).
> > 
> > The method I use is probably slightly confusing, but it works ;-)
...
> > The patch also adds some tests that verify the in-profile
> > deduplication.
> This looks good; I've got one suggestion, though; in both
> delete_duplicates and delete_in_profile_duplicates the 'deleted'
> variable is a list of deleted rules, but no use is made of their
> contents. deleted could be turned into an integer, and the .append()
> calls turned into += 1 and returning the value directly.

That's there for historical reasons[tm], and you are right that just 
counting is enough.

> Thanks for the tests; I think one more case would be nice to add,
> something like:
> 
>   rules = [ 'audit capability chown', 'deny capability chown' ]
> 
> simply because I'm not confident at what the answer should be. Should
> it be "audit deny capability chown" or "deny capability chown"? It'd
> be nice to codify what the result should be.

The tools will keep both rules. Merging them to "audit deny capability 
chown" would also be correct, but I doubt I want to implement that ;-)

Added as test_delete_duplicates_in_profile_04().


Here's the updated patch:

[ 42-in-profile-deduplication.diff ]

=== modified file utils/apparmor/rule/__init__.py
--- utils/apparmor/rule/__init__.py     2015-04-24 21:47:02.319685749 +0200
+++ utils/apparmor/rule/__init__.py     2015-04-24 21:50:18.690953604 +0200
@@ -230,15 +230,39 @@
 
     def delete_duplicates(self, include_rules):
         '''Delete duplicate rules.
-           include_rules must be a *_rules object'''
-        deleted = []
-        if include_rules:  # avoid breakage until we have a proper function to ensure all profiles contain all *_rules objects
+           include_rules must be a *_rules object or None'''
+
+        deleted = 0
+
+        # delete rules that are covered by include files
+        if include_rules:
             for rule in self.rules:
                 if include_rules.is_covered(rule, True, True):
                     self.delete(rule)
-                    deleted.append(rule)
+                    deleted += 1
+
+        # de-duplicate rules inside the profile
+        deleted += self.delete_in_profile_duplicates()
+        self.rules.reverse()
+        deleted += self.delete_in_profile_duplicates()  # search again in reverse order - this will find the remaining duplicates
+        self.rules.reverse()  # restore original order for raw output
+
+        return deleted
+
+    def delete_in_profile_duplicates(self):
+        '''Delete duplicate rules inside a profile'''
+
+        deleted = 0
+        oldrules = self.rules
+        self.rules = []
+
+        for rule in oldrules:
+            if not self.is_covered(rule, True, False):
+                self.rules.append(rule)
+            else:
+                deleted += 1
 
-        return len(deleted)
+        return deleted
 
     def get_glob_ext(self, path_or_rule):
         '''returns the next possible glob with extension (for file rules only).
=== modified file utils/test/test-capability.py
--- utils/test/test-capability.py       2015-04-24 21:47:02.320685689 +0200
+++ utils/test/test-capability.py       2015-04-24 21:58:20.069631045 +0200
@@ -855,5 +855,118 @@
         self.assertEqual(expected_clean, self.ruleset.get_clean(1))
 
 
+    def _check_test_delete_duplicates_in_profile(self, rules, expected_raw, expected_clean, expected_deleted):
+        obj = CapabilityRuleset()
+
+        for rule in rules:
+            obj.add(CapabilityRule.parse(rule))
+
+        deleted = obj.delete_duplicates(None)
+
+        self.assertEqual(expected_raw, obj.get_raw(1))
+        self.assertEqual(expected_clean, obj.get_clean(1))
+        self.assertEqual(deleted, expected_deleted)
+
+
+    def test_delete_duplicates_in_profile_01(self):
+        rules = [
+            'audit capability chown,',
+            'audit capability,',
+            'capability dac_override,',
+        ]
+
+        expected_raw = [
+            '  audit capability,',
+            '',
+        ]
+
+        expected_clean = [
+            '  audit capability,',
+            '',
+        ]
+
+        expected_deleted = 2
+
+        self._check_test_delete_duplicates_in_profile(rules, expected_raw, expected_clean, expected_deleted)
+
+    def test_delete_duplicates_in_profile_02(self):
+        rules = [
+            'audit capability chown,',
+            'audit capability,',
+            'audit capability dac_override,',
+            'capability ,',
+            'audit capability ,',
+        ]
+
+        expected_raw = [
+            '  audit capability,',
+            '',
+        ]
+
+        expected_clean = [
+            '  audit capability,',
+            '',
+        ]
+
+        expected_deleted = 4
+
+        self._check_test_delete_duplicates_in_profile(rules, expected_raw, expected_clean, expected_deleted)
+
+    def test_delete_duplicates_in_profile_03(self):
+        rules = [
+            'audit capability chown,',
+            'capability dac_override,',
+            'deny capability dac_override,',
+            'capability dac_override,',
+            'audit capability chown,',
+            'deny capability chown,',
+            'audit deny capability chown,',
+            'capability,',
+            'audit capability,',
+        ]
+
+        expected_raw = [
+            '  deny capability dac_override,',
+            '  audit deny capability chown,',
+            '  audit capability,',
+            '',
+        ]
+
+        expected_clean = [
+            '  audit deny capability chown,',
+            '  deny capability dac_override,',
+            '',
+            '  audit capability,',
+            '',
+        ]
+
+        expected_deleted = 6
+
+        self._check_test_delete_duplicates_in_profile(rules, expected_raw, expected_clean, expected_deleted)
+
+    def test_delete_duplicates_in_profile_04(self):
+        rules = [
+            'audit capability chown,',
+            'deny capability chown,',
+        ]
+
+        expected_raw = [
+            '  audit capability chown,',
+            '  deny capability chown,',
+            '',
+        ]
+
+        expected_clean = [
+            '  deny capability chown,',
+            '',
+            '  audit capability chown,',
+            '',
+        ]
+
+        expected_deleted = 0
+
+        self._check_test_delete_duplicates_in_profile(rules, expected_raw, expected_clean, expected_deleted)
+
+
 if __name__ == "__main__":
     unittest.main(verbosity=2)



Regards,

Christian Boltz
-- 
Maybe the next openSUSE conference should have a mandatory group hug 
after all... ;-) [Jos Poortvliet in opensuse-factory]