[apparmor] [patch 3/3] use capability rule class in aa.py and cleanprof.py
Steve Beattie
steve at nxnw.org
Wed Dec 3 18:39:13 UTC 2014
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 <steve at nxnw.org>
---
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
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141203/d86edaed/attachment.pgp>
More information about the AppArmor
mailing list