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. > > 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? 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 <[email protected]> --- utils/apparmor/rule/__init__.py | 44 ++++++++++++++++++++++++++++++++++---- utils/apparmor/rule/capability.py | 13 ++++------- 2 files changed, 45 insertions(+), 12 deletions(-) Index: b/utils/apparmor/rule/__init__.py =================================================================== --- a/utils/apparmor/rule/__init__.py +++ b/utils/apparmor/rule/__init__.py @@ -23,18 +23,44 @@ _ = init_translation() class BaseRule(object): '''Base class to handle and store a single rule''' + # type specific rules should inherit from this class. + # Methods that subclasses need to implement: + # __init__ + # _parse(cls, raw_rule) (as a class method) + # - parses a raw rule and returns an object of the Rule subclass + # get_clean(depth) + # - return rule in clean format + # is_covered(self, other_rule) + # - check if other_rule is covered by this rule (i.e. is a + # subset of this rule's permissions) + # is_equal_localvars(self, other_rule) + # - equality check for the rule-specific fields + def __init__(self, audit=False, deny=False, allow_keyword=False, - comment='', log_event=None, raw_rule=''): + comment='', log_event=None): '''initialize variables needed by all rule types''' self.audit = audit self.deny = deny self.allow_keyword = allow_keyword - self.comment = comment - - self.raw_rule = raw_rule.strip() if raw_rule else None self.log_event = log_event + # Set only in the parse() class method + self.raw_rule = None + + @classmethod + def parse(cls, raw_rule): + rule = cls._parse(raw_rule) + rule.raw_rule = raw_rule.strip() + return rule + + # @abstractmethod FIXME - uncomment when python3 only + @classmethod + def _parse(cls, raw_rule): + '''returns a Rule object created from parsing the raw rule. + required to be implemented by subclasses; raise exception if not''' + raise AppArmorBug("'%s' needs to implement _parse(), but didn't" % (str(cls))) + def get_raw(self, depth=0): '''return raw rule (with original formatting, and leading whitespace in the depth parameter)''' if self.raw_rule: @@ -58,6 +84,16 @@ class BaseRule(object): 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(cls))) + + # @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))) + def modifiers_str(self): '''return the allow/deny and audit keyword as string, including whitespace''' Index: b/utils/apparmor/rule/capability.py =================================================================== --- a/utils/apparmor/rule/capability.py +++ b/utils/apparmor/rule/capability.py @@ -35,13 +35,12 @@ class CapabilityRule(BaseRule): ALL = __CapabilityAll def __init__(self, cap_list, audit=False, deny=False, allow_keyword=False, - comment='', log_event=None, raw_rule=None): + comment='', log_event=None): super(CapabilityRule, self).__init__(audit=audit, deny=deny, allow_keyword=allow_keyword, comment=comment, - log_event=log_event, - raw_rule=raw_rule) + log_event=log_event) # Because we support having multiple caps in one rule, # initializer needs to accept a list of caps. self.all_caps = False @@ -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) def get_clean(self, depth=0): @@ -133,8 +132,6 @@ def parse_capability(raw_rule): if not matches: raise AppArmorException(_("Invalid capability rule '%s'") % raw_rule) - raw_rule = raw_rule.strip() - audit, deny, allow_keyword, comment = parse_modifiers(matches) capability = [] @@ -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) -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
