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

[Seth Arnold]

On Sun, Apr 12, 2015 at 03:29:44AM +0200, Christian Boltz wrote:
> Hello,
> 
> 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 ;-)
> 
> (1) Store the current rules in oldrules, empty self.rules and then
>     self.is_covered()-loop over oldrules and add only the needed rules
>     back to self.rules. After this step, some, but not all duplicates
>     are removed (depends on the rule order inside the profile file).
> 
> (2) Reverse the order of self.rules
> 
> (3) Do step (1) again. This deletes the remaining duplicates.
> 
> (3) Reverse self.rules again to get the original order of the
>     (remaining) rules back.
> 
> If you are still confused, look at the examples below ;-)
> 
> 
> 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.

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.

Thanks

> [ 42-in-profile-deduplication.diff ]
> 
> === modified file 'utils/apparmor/rule/__init__.py'
> --- utils/apparmor/rule/__init__.py     2015-01-16 13:59:49 +0000
> +++ utils/apparmor/rule/__init__.py     2015-04-12 01:15:34 +0000
> @@ -227,14 +230,40 @@
>  
>      def delete_duplicates(self, include_rules):
>          '''Delete duplicate rules.
> -           include_rules must be a *_rules object'''
> +           include_rules must be a *_rules object or None'''
> +
>          deleted = []
> -        if include_rules:  # avoid breakage until we have a proper function to ensure all profiles contain all *_rules objects
> +
> +        # 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)
>  
> +        num_deleted = len(deleted)
> +
> +        # de-duplicate rules inside the profile
> +        num_deleted += self.delete_in_profile_duplicates()
> +        self.rules.reverse()
> +        num_deleted += self.delete_in_profile_duplicates()  # search again in reverse order - this will find more duplicates
> +        self.rules.reverse()  # restore original order for raw output
> +
> +        return num_deleted
> +
> +    def delete_in_profile_duplicates(self):
> +        '''Delete duplicate rules inside a profile'''
> +
> +        deleted = []
> +        oldrules = self.rules
> +        self.rules = []
> +
> +        for rule in oldrules:
> +            if not self.is_covered(rule, True, False):
> +                self.rules.append(rule)
> +            else:
> +                deleted.append(rule)
> +
>          return len(deleted)
>  
>      def get_glob_ext(self, path_or_rule):
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150423/16c88e83/attachment.pgp>