[apparmor] [patch] Implement in-profile de-duplication in BaseRuleset
Kshitij Gupta
kgupta8592 at gmail.com
Fri Apr 24 21:04:16 UTC 2015
Hello,
On 25-Apr-2015 1:38 am, "Christian Boltz" <apparmor at cboltz.de> wrote:
>
> 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
Hmm a couple of reasons to use the list of deleted rules included being
able to view/print/log the deleted rules for debugging and ability to maybe
show all the deleted rules for the profile. Though there probably are
better and easier ways to do both without incurring this overhead(?).
Thanks.
Regards,
Kshitij Gupta
> 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 at lists.ubuntu.com
> Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150425/49bffcab/attachment-0001.html>
More information about the AppArmor
mailing list