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] -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor