[apparmor] [1/4] Add ChangeProfileRule and ChangeProfileRuleset classes

[Christian Boltz]

Hello,

Am Dienstag, 26. Mai 2015 schrieb Seth Arnold:
> On Sat, May 09, 2015 at 10:35:54PM +0200, Christian Boltz wrote:
> 
> Hi Christian, just some quick notes from a first read...
> 
> > [ 01-add-ChangeProfileRule.diff ]
> 
> ...
> 
> > +
> > +class ChangeProfileRule(BaseRule):
> > +    '''Class to handle and store a single network rule'''
> > +
> 
> "network rule" is a holdover..

Oops ;-)

I changed that in the patch locally (but won't resend it just for this).

> > +    def __init__(self, execcond, targetprofile, audit=False,
> > deny=False, allow_keyword=False, +                 comment='',
> > log_event=None):
> > +
> > +        '''
> > +            CHANGE_PROFILE RULE = 'change_profile' [ EXEC COND ] [
> > -> PROGRAMCHILD ] +        '''
> > +
> > +        super(ChangeProfileRule, self).__init__(audit=audit,
> > deny=deny, +                                            
> > allow_keyword=allow_keyword, +                                     
> >        comment=comment, +                                          
> >   log_event=log_event) +
> > +        self.execcond = None
> > +        self.all_execconds = False
> > +        if execcond == ChangeProfileRule.ALL:
> > +            self.all_execconds = True
> > +        elif type(execcond) == str:
> > +            if execcond.strip():
> > +                if execcond.startswith('/'):
> > +                    self.execcond = execcond
> > +                else:
> > +                    raise AppArmorException('Exec condition in
> > change_profile rule does not start with /: %s' % str(execcond)) +  
> >          else:
> > +                raise AppArmorBug('Empty exec condition in
> > change_profile rule') +        else:
> > +            raise AppArmorBug('Passed unknown object to
> > ChangeProfileRule: %s' % str(execcond))
> I'm quite confused by the all_execconds portions of this class; "if"
> statements nested three levels deep, with multiple else clauses, is a
> lot to digest.

I'm not too surprised ;-)

The reason for doing the "top-level" if condition that way is to be 
consistent with other classes, which have similar checks ("if ALL: / 
elif type() == str: / else:")

Maybe we could change some checks to "if not ...: raise ..." to save one 
nesting level, but I'm not sure if saving one nesting level outweights 
having several "not" in the conditions.

<online shop mode>
    People who liked the if conditions in ChangeProfileRule __init__()
    will *love* the if conditions in the RlimitRule __init__()
    (patch 41)
</online shop mode>

> I had trouble seeing what all_execconds buys compared
> to doing the other comparisons with execcond directly.

Ask Steve ;-) - the separate all_* vars are something he wanted for 
CapabilityRule. (IIRC one reason was to avoid to have lots of
"if self.foo == FooRule.ALL" checks.)

Now I'm just following the style of CapabilityRule ;-)

> There's also mixed parsing here, with strip() 

That's basically a "contains non-whitespace chars" test because 
whitespace-only would result in ALL permissions.

> and startswith(), and
> some extra parsing going on in the _parse() function. I wonder if
> there is a better division of parsing available.

__init__() just does checks and saves the values in class variables, but 
doesn't do any parsing.

_parse() takes a line from a profile as parameter and then converts it 
to a *Rule object, which effectively means calling __init__()


Regards,

Christian Boltz
-- 
Systemd is probably the number one "bang your head against the wall"
technology of the last few years
[Caio on http://seifesrants.blogspot.de/2013/05/the-systemd-journal-is-broken-piece-of.html]