[apparmor] [patch] rule class - split out common parts from is_covered()

[Christian Boltz]

Hello,

Am Dienstag, 13. Januar 2015 schrieb Steve Beattie:
> On Fri, Dec 19, 2014 at 12:24:02AM +0100, Christian Boltz wrote:
> > this patch splits is_covered() in capability.py into
> > - is_covered_localparts() for rule-specific code
> > - is_covered() for common code - located in __init__.py
> 
> I'm not keen on the naming convention xxxxx_localvars() that the code
> actually uses, the xxxxx_localparts() listed above is makes me cringe
> slightly less.

Those (internal) function names are just technical details - I can 
easily change that to ..._localparts().

> > The object type comparison now uses type(self) and a slightly
> > different error message to make it usable everywhere.
> > 
> > It also renames rule_obj to other_rule which is more self-explaining
> > (inspired by the parameter name in the is_covered() dummy in
> > __init__.py).
> > 
> > 
> > [ rule_split_is_covered.diff ]
> > 
> > === modified file 'utils/apparmor/rule/__init__.py'
> > --- utils/apparmor/rule/__init__.py     2014-12-16 22:17:33 +0000
> > +++ utils/apparmor/rule/__init__.py     2014-12-18 23:16:13 +0000
> > @@ -68,6 +68,29 @@
> > 
> >          else:
> >              return self.get_clean(depth)
> > 
> > +    def is_covered(self, other_rule, check_allow_deny=True,
> > check_audit=False): +        '''check if other_rule is covered by
> > this rule object''' +
> > +        if not type(other_rule) == type(self):
> > +            raise AppArmorBug('Passes %s instead of %s' %
> > (str(other_rule),self.__class__.__name__)) +
> > +        if check_allow_deny and self.deny != other_rule.deny:
> > +            return False
> > +
> > +        if check_audit and other_rule.audit != self.audit:
> > +            return False
> > +
> > +        if other_rule.audit and not self.audit:
> > +            return False
> 
> I recognize that you are merely pushing these checks around, but do
> you recall why we do the second audit test after the first? I don't
> quite get the reasoning...

if check_audit and other_rule.audit != self.audit:
    exact check if audit flags are equal, so you get exactly the same 
    rule and exactly the same logging. Only checked if the check_audit
    flag is given.

if other_rule.audit and not self.audit:
    released check - only hits if other_rule has audit flag set to make 
    sure we get audit entries in the log - but also "says" things like
    "foo is covered by audit foo".

> > +
> > +        # still here? -> then the common part is covered, check
> > rule-specific things now +        return
> > self.is_covered_localvars(other_rule, check_allow_deny,
> > check_audit) +
> > +    # @abstractmethod  FIXME - uncomment when python3 only
> 
> > +    def is_covered_localvars(self, other_rule, 
check_allow_deny=True, check_audit=False):
> If check_allow_deny and check_audit are verified in the BaseRule
> method, is there a reason to pass them on to the subclass version of
> the method, since they should be irrelevant to them?

Good question, in theory they are really superfluous.

My reason for handing them over is to have the same set of parameters in 
the parent and child function.

> > +        '''check if the rule-specific parts of other_rule is
> > covered by this rule object''' +        raise AppArmorBug("'%s'
> > needs to implement is_covered_localvars(), but didn't" %
> > (str(self))) +
> > 
> >      def is_equal(self, rule_obj, strict=False):
> >          '''compare if rule_obj == self
> >          
> >             Calls is_equal_localvars() to compare rule-specific
> >             variables'''
> > 
> > @@ -85,11 +108,6 @@
> > 
> >          return self.is_equal_localvars(rule_obj)
> >      
> >      # @abstractmethod  FIXME - uncomment when python3 only
> > 
> > -    def is_covered(self, other_rule, check_allow_deny=True,
> > check_audit=False): -        '''check if other_rule is covered by
> > this rule object''' -        raise AppArmorBug("'%s' needs to
> > implement is_covered(), but didn't" % (str(self))) -
> > -    # @abstractmethod  FIXME - uncomment when python3 only
> > 
> >      def is_equal_localvars(self, other_rule):
> >          '''compare if rule-specific variables are equal'''
> >          raise AppArmorBug("'%s' needs to implement
> >          is_equal_localvars(), but didn't" % (str(self)))> 
> > === modified file 'utils/apparmor/rule/capability.py'
> > --- utils/apparmor/rule/capability.py   2014-12-16 22:17:33 +0000
> > +++ utils/apparmor/rule/capability.py   2014-12-18 22:50:11 +0000
> > @@ -95,30 +95,18 @@
> > 
> >              else:
> >                  raise AppArmorBug("Empty capability rule")
> > 
> > -    def is_covered(self, rule_obj, check_allow_deny=True,
> > check_audit=False): -        '''check if rule_obj is covered by
> > this rule object''' -
> > -        if not type(rule_obj) == CapabilityRule:
> > -            raise AppArmorBug('Passes non-capability rule: %s' %
> > str(rule_obj)) -
> > -        if check_allow_deny and self.deny != rule_obj.deny:
> > -            return False
> > +    def is_covered_localvars(self, other_rule,
> > check_allow_deny=True, check_audit=False): +        '''check if
> > other_rule is covered by this rule object'''
> > 
> > -        if not rule_obj.capability and not rule_obj.all_caps:
> > 
> > +        if not other_rule.capability and not other_rule.all_caps:
> >              raise AppArmorBug('No capability specified')
> >          
> >          if not self.all_caps:
> > -            if rule_obj.all_caps:
> > 
> > +            if other_rule.all_caps:
> >                  return False
> > 
> > -            if not rule_obj.capability.issubset(self.capability):
> > 
> > +            if not other_rule.capability.issubset(self.capability):
> >                  return False
> > 
> > -        if check_audit and rule_obj.audit != self.audit:
> > -            return False
> > -
> > -        if rule_obj.audit and not self.audit:
> > -            return False
> > -
> > 
> >          # still here? -> then it is covered
> >          return True
> > 
> > Regards,
> > 
> > Christian Boltz
Regards,

Christian Boltz
-- 
Es gibt in Afrika einen Stamm, die stehen auf einem Bein. Das ist deren
Standart. Und weil sie immer so stehen, ist deren Standart bei denen
Standard. [Steffen Schmidt in de.comm.infosystems.www.pages.misc]