On Mon, Dec 01, 2014 at 07:51:16PM +0100, Christian Boltz wrote:
> Am Sonntag, 30. November 2014 schrieb Christian Boltz:
> > Let me warn you that your __init__() also has a regression when
> > compared with my set_* functions - imagine someone calls it with a
> > raw_rule that completely differs from the other parameters, like
> >
> > cap_rule = CapabilityRule("chown", raw_rule="capability,")
> >
> > The result of cap_rule.get_raw() will be a very permissive profile ;-)
> > but all checks like is_equal() and is_covered() will assume cap_rule
> > only allows chown.
>
> As discussed on IRC, this can be solved:
>
> <cboltz> sbeattie: how would you like it if CapabilityRule __init__ allows
> two different ways to hand over the data?
> <cboltz> a) foo = CapabilityRule('chown') (as in your proposal)
> <cboltz> b) foo = CapabilityRule(raw_rule='capability chown,') (and then
> parse raw_rule)
> <cboltz> it would make __init__ a bit more complex -
> <cboltz> if raw_rule:
> <cboltz> set values based on parse_capability()
> <cboltz> else:
> <cboltz> do what "your" __init__ does now
> <sbeattie> cboltz: I think I could be okay with that; I thought you wanted to
> avoid doing stuff like that.
> <cboltz> well, I still prefer it over the "external" parse_capability
> function ;-)
> <cboltz> (and it would fix the problem that raw_rule can be completely
> different from everything else)
> <cboltz> will you update your patch, or do you want a patch from me on top of
> yours? ;-)
> * cboltz wonders how many "patch on top of patch" levels we need until it
> starts to get confusing
> <sbeattie> cboltz: I can do it, but not right this minute, so can you send
> the request as a followup to your feedback email so I don't lose track of it.
> <cboltz> no problem, I'll just paste the IRC log to a mail ;-)
> <sbeattie> awe. some.
>
> 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). Signed-off-by: Steve Beattie <[email protected]> --- utils/apparmor/rule/__init__.py | 21 +++++++++++++++++---- utils/apparmor/rule/capability.py | 13 +++++-------- 2 files changed, 22 insertions(+), 12 deletions(-) Index: b/utils/apparmor/rule/__init__.py =================================================================== --- a/utils/apparmor/rule/__init__.py +++ b/utils/apparmor/rule/__init__.py @@ -24,17 +24,30 @@ class BaseRule(object): '''Base class to handle and store a single rule''' 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: 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): @@ -132,8 +131,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 = [] @@ -146,5 +143,5 @@ 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
