[apparmor] [patch 4/3] hide raw_rule within parse class method

[Christian Boltz]

Hello,

Am Montag, 8. Dezember 2014 schrieb Steve Beattie:
> On Wed, Dec 03, 2014 at 10:39:13AM -0800, Steve Beattie wrote:
> > > For readabiliy, it might be a good idea to keep parse_capability()
> > > as a separate function inside the CapabilityRule class and call
> > > it from __init__.
> > 
> > So, I started down this path, but I really didn't like how
> > convoluted
> > it was making the logic in __init__; it required verifying that one
> > and only one of cap_list and raw_rule were passed and splitting up
> > the parse_capability function a bit at the very least.
> > 
> > I came up with another solution, for which I've attached a patch. I
> > moved the parse() entry point to the base class, which then calls
> > the intended-for-internal-consumption-only class method _parse(). I
> > removed the raw_rule argument to both __init__ methods and haver
> > BaseRule.parse() set raw_rule directly.
> > 
> > Thus, the two intended entry points are __init__(), where you're
> > expected to have already dissected whatever rule into digestible
> > components *or* the Rule.parse() class method, where only a raw_rule
> > is given. In this way, using the obvious methods of interacting
> > with the CapabilityRule class, you can't have a mismatch between
> > the stored capabilities and the raw_rule unless there's a bug in the
> > parsing function *or* you've gone digging into the internals of the
> > object.

Not exactly what I had in mind, but I understand why you do it this way.

> > And if you do go around getting grotty with the class fields like
> > raw_rule directly, (1) it's not recommended, and (2) you are
> > expected
> > to know what you're doing (e.g. you're a test script).
> 
> Christian, any comments on this?

Yes ;-)  - sorry for the delay!

> I've updated the patch to add some comments to the base class about
> what rule subclasses need to implement.
> 
> Signed-off-by: Steve Beattie <steve at nxnw.org>

> Index: b/utils/apparmor/rule/__init__.py
> ===================================================================
> --- a/utils/apparmor/rule/__init__.py
> +++ b/utils/apparmor/rule/__init__.py
...
> +    # @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(cls))) 

You'll get an exception here, but not the one you expect - cls is not 
defined here ;-)

> +    # @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(cls))) 

Same here - cls is undefined.

> Index: b/utils/apparmor/rule/capability.py
> ===================================================================
> --- a/utils/apparmor/rule/capability.py
> +++ b/utils/apparmor/rule/capability.py
...
> @@ -61,8 +60,8 @@ class CapabilityRule(BaseRule):
>                  if len(cap.strip()) == 0:
>                      raise AppArmorBug('Passed empty capability to
> CapabilityRule: %s' % str(cap_list))
> 
> -    @staticmethod
> -    def parse(raw_rule):
> +    @classmethod
> +    def _parse(cls, raw_rule):
>          return parse_capability(raw_rule)

parse_capability() is (IIRC) only called here - feel free to integrate 
it directly in _parse()

> @@ -147,4 +144,4 @@ def parse_capability(raw_rule):
> 
>      return CapabilityRule(capability, audit=audit, deny=deny,
>                            allow_keyword=allow_keyword,
> -                          comment=comment, raw_rule=raw_rule)
> +                          comment=comment)

This change means you no longer set raw_rule. Please change this to

    rule = CapabilityRule(......)
    rule.raw_rule = raw_rule
    return rule


With these details fixed,
Acked-by: Christian Boltz <apparmor at cboltz.de>


Regards,

Christian Boltz
-- 
Neue Windows-Tastatur seit heute zu kaufen.
Hier ein erstes Bild:
,-----,   ,-----,   ,------,
| Str |   | Alt |   | Entf |
'-----'   '-----'   '------'