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

[Christian Boltz]

Hello,

Am Dienstag, 18. November 2014 schrieb Steve Beattie:
> 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:
...
> > 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.

Something like that (IIRC):

class capability_rules(base_rules):
    fullprofile = None
    includes = None
    def __init__(self, fullprofile, includes):
        self.fullprofile = fullprofile
        self.includes = includes

then use
    foo = capability_rules(aa[profile][hat], include)

That said - keeping the includes outside isn't as bad as I thought, so 
don't worry about that too much ;-)

The only "problem" with this way is that we need to keep the functions 
that check includes for covered rules outside - see aa.py 
profile_known_capability() and match_cap_include().

BTW: my long-term plan is to have profile_known_rule() and 
match_include() functions that work for all 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.

Yes, audit and deny (and some more) are defined and stored in base_rule.

...
> >     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.

I'm using it from aa.py when writing the profile in non-clean mode, and 
that code needs to clear the rules it already wrote. I could also use
    aa[profile][hat]['capability'] = capability_rules()
there. However, using
    aa[profile][hat]['capability'].delete_all_rules()
has a big advantage: We can replace 'capability' with a variable and 
easily loop over all rule types. You can't do that with 
    ... = capability_rules() ;-)

That said - the code to write a profile in non-clean mode (aka "(V)iew 
changes") is <diplomatic>not really nice</diplomatic>, but that's 
another rewrite ;-)

> 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).

__init() is needed - try running the testsuite without it and you'll 
know the reason ;-)

(and yes, I was also surprised that I explicitely need self.rules = [] 
in __init__() - what python does without the __init__() isn't what I'd 
expect)

> >     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?

rawrule is a string - basically a line from a profile (for example 
    capability      chown    ,      #  I       love       spaces
is a rawrule)

> >     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.

I always have a set of ..._raw, ..._obj and ..._log. The difference is 
the parameter type - a rawrule for _raw, a rule object like 
capability_rule (without "s"!)  for _obj and a parsed log event (as 
returned by libapparmor logparser) for _log.

BTW: is ..._rule vs ..._rules ok or should we use a name that differs 
more? (If yes, any idea?)

> >     def delete_duplicates(self, inccaps):
> >         '''Delete duplicate rules.
> 
> What is the inccaps argument here?

The capabilities of an include file (include[file][file]['capability'] 
in aa.py IIRC). I should probably rename inccaps to include_rules to 
make the name fit for all rule types.

> >     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?

At the moment, my plan is to return a string (basically a rawrule), but 
that's something we can change if you have a better idea.

And yes, "capability," would be a valid glob - and also something I 
wouldn't want to have in my profiles ;-)

BTW: get_glob_ext() is for "Glob with (E)xt" - for capabilities, only 
get_glob() is available.

> > 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.)

Because someone had the idea to allow
    capability chown chgrp audit_write,
in the profiles ;-)

Until now, the utils just parsed this as 
    "capability"     "chown chgrp audit_write"
but that makes finding duplicates quite hard ;-)

...
> > 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.

This little helper function allowed me to move several functions to the 
base_rules class (the only difference for usage with other rule types 
was the class name of the rule object, so it made sense to split it out 
to the new_rule() function)

> >     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.?

Depends on how you want to use the result ;-)

For usage with aa-logprof, returning only one ("the next") glob fits the 
user interface perfectly. If you have different use cases (or prefer a 
different implementation way), tell me ;-)


Regards,

Christian Boltz
-- 
Am besten im Targa-Format,  oder Geotiff,  oder HandshakeCT, oder so.
Bloß nichts komprimiertes! Wohlmöglich entpackt sich die Datei in der
Leitung, und dann ist das Kabel im Eimer.       [Ratti in suse-linux]