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

[Seth Arnold]

On Fri, Apr 24, 2015 at 10:08:16PM +0200, Christian Boltz 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 
> 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 ]

Thanks!

Acked-by: Seth Arnold <seth.arnold at canonical.com>

> 
> === 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)
> 
-------------- 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/20150424/5d2e16ba/attachment.pgp>