[apparmor] [patch 1/3] add base and capability rule classes

[Steve Beattie]

Hi Christian,

Thanks for starting this work. I haven't started looking at the code,
but wanted to ask questions and comment about the proposed classes.

On Sat, Nov 15, 2014 at 11:43:51PM +0100, Christian Boltz wrote:
> This patch adds four classes - two "base" classes and two specific for 
> capabilities:
> 
> utils/apparmor/rule/__init__.py:
> 
>     class base_rule(object):
>         Base class to handle and store a single rule
> 
>     class base_rules(object):
>         Base class to handle and store a collection of rules
> 
> utils/apparmor/rule/capability.py:
> 
>     class capability_rule(base_rule):
>         Class to handle and store a single capability rule
> 
>     class capability_rules(base_rules):
>         Class to handle and store a collection of capability rules
> 
> 
> Most of the code is in the base classes (in __init__.py) - that's code 
> that will be reused by each rule class.
> 
> capability.py contains only code that is specific to capabilities.

That all seems sensible.

> The layout of the base_rules and capability_rules class is not too
> different from the proposal I sent some weeks ago. 
> 
> The biggest difference is that the class doesn't store the full profile
> and include list. When I tried to do this, aa-logprof started to eat 
> *lots of* memory before I Ctrl-C'd it in a deepcopy() operation.
> Either we find a way to implement this in a sane way that doesn't eat
> memory, or we just keep it out ;-)

Do you have a code snippet of how you went about this? Keeping
references to things should be relatively lightweight. Perhaps it's
an artifact of the specific implementation of the global hasher thingy.

> I didn't implement the functions to propose rules yet - that's something 
> for the second round. The interesting question is if we can do this 
> inside the class without having the list of include files inside the 
> class.
> 
> The storage for single rules is more different - initially I planned to
> use a dict at least for the simple rule types like capability, but it
> turned out that using a class has some advantages.

Yeah. Even if we did something 'clever' with capabilities, given that
its a small finite set of permissions, it would not be a good model for
the other rule types.

> Here's the list of functions in each class:
> 
> utils/apparmor/rule/__init__.py:
> 
> class base_rule(object):
>     '''Base class to handle and store a single rule'''

Are the fields that you expect to store in each rule? 'audit' and 'deny'
seem like obvious candidates.

>     def get_raw(self, depth):
>         '''return raw rule (with original formatting, and leading whitespace in the depth parameter)'''
> 
>     def audit_allow_str(self):
>         '''return the allow/deny and audit keyword as string, including whitespace'''
> 
>     def parse_audit_allow(self, matches):
>         '''returns audit, deny, allow_keyword and comment from the matches object
> 
> class base_rules(object):
>     '''Base class to handle and store a collection of rules'''
> 
>     def __init__(self):
>         self.delete_all_rules()
>
>     def delete_all_rules(self):
>         self.rules = []

Why is delete_all_rules() necessary? __init__() should probably just do
the initialization of self.rules directly, and we really shouldn't be
re-using an object to encapsulate a different set of rules.

And realistically, if you just declare 'rules = []' as a field in the
class, then neither function is necessary (though it wouldn't surprise
me if we needed __init__() for other reasons).

>     def add_raw(self, rawrule):
>         '''parse rawrule (from profile file) and store it in a structured way'''

Is rawrule a string or a complex data structure that contains
back-reference information to the file/line it came from?

>     def get_raw(self, depth):
>         '''return all raw rules (if possible/not modified in their original formatting).
> 
>     def get_clean(self, depth):
>         '''return all rules (in clean/default formatting)
> 
>     def covered_obj(self, rule_obj, check_allow_deny = True, check_audit = False):
>         '''return True if rule_obj is covered by existing rules, otherwise False'''
> 
>     def covered_log(self, parsed_log_event, check_allow_deny = True, check_audit = False):
>         '''return True if parsed_log_event is covered by existing rules, otherwise False'''
> 
>     def covered_raw(self, rawrule, check_allow_deny = True, check_audit = False):
>         '''return True if rawrule is covered by existing rules, otherwise False'''
> 
>     def delete_obj(self, rule_obj):
>         '''Delete rule_obj from rules'''
> 
>     def delete_raw(self, rawrule):
>         '''Delete rawrule from rules'''

Some of these feel like duplication, or overlapping of purpose.

>     def delete_duplicates(self, inccaps):
>         '''Delete duplicate rules.

What is the inccaps argument here?

>     def get_glob_ext(self, path_or_rule):
>         '''returns the next possible glob with extension (for file rules only).

When we come around to dbus and some of the other rule types, it's
likely they will also have meaningful results. What's the return
type/value for if there's no meaningful glob? Would 'capability,'
(*shudder*) be a valid glob for capabilities?

> utils/apparmor/rule/capability.py:
> 
> class capability_rule(base_rule):
>     '''Class to handle and store a single capability rule'''
> 
>     def __init__(self):
>         self.capability = []

Why is it a list, for a single capability? (And again, just declare the
field with the initial value.)

>     def get_clean(self, depth):
>         '''return rule (in clean/default formatting)'''
> 
>     def set_raw(self, rawrule):
>         '''parse and store rawrule'''
> 
>     def set_log(self, parsed_log_event):
>         '''parse and store log event'''
> 
>     def covered(self, rule_obj, check_allow_deny = True, check_audit = False):
>         '''check if rule_obj is covered by this rule object'''
> 
> class capability_rules(base_rules):
>     '''Class to handle and store a collection of capability rules'''
> 
>     def new_rule(self):
>         '''tiny helper function that allows to keep several functions to parent class'''

I... don't quite parse that, but perhaps I need to look at code to see
the purpose.

>     def get_glob(self, path_or_rule):
>         '''Return the next possible glob. For capability rules, that's always "capability," (all capabilities)'''

Seems like perhaps there should be just a get_globs() function,
which returns a list of globs/glob extensions, etc.?

Thanks again.

-- 
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/20141118/5995a8d5/attachment.pgp>