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

[Steve Beattie]

On Thu, May 28, 2015 at 09:51:31PM +0200, Christian Boltz wrote:
> Am Mittwoch, 27. Mai 2015 schrieb Steve Beattie:
> > This is problematic in the situation where paths begin with an @{}
> > variable (e.g. 'change_profile @{MY_PROGRAM} -> my_program_profile,').
> 
> We can prepare for variables nevertheless ;-)
> 
> For the moment, I added an ... or execcond.startswith('@')
> 
> On the long term, maybe a regex check would be better - but that's 
> something for a follow-up patch.

Sure, ideally the specifics of the validity of an fs path would be
hidden within a function or method call.

> Given that there are variables, *, **, ?, [...] and {...} (did I miss 
> something?), maybe we'll even end up with a separate class for paths 
> that can be used in various rule types.

The parser only accepts path rules that begin with / or @{. But yeah,
there may need to be a class for path elements.

> > > +            else:
> > > +                raise AppArmorBug('Empty exec condition in
> > > change_profile rule') +        else:
> > > +            raise AppArmorBug('Passed unknown object to
> > > ChangeProfileRule: %s' % str(execcond))
> > That said, I also agree with Seth's comment, it might be nice to
> > restructure the if else clauses like so:
> 
> [...quoting of overlong lines destroyed by KMail...]
> 
> > Otherwise, this is looking okay.
> 
> :-)
> 
> 
> Here's the updated patch - changes are
> - s/network/change_profile/ in a comment
> - the if conditions discussed above changed to avoid a nesting level:
> 
> +        if execcond == ChangeProfileRule.ALL:
> +            self.all_execconds = True
> +        elif type(execcond) == str:
> +            if not execcond.strip():
> +                raise AppArmorBug('Empty exec condition in change_profile rule')
> +            elif execcond.startswith('/') or execcond.startswith('@'):
> +                self.execcond = execcond
> +            else:
> +                raise AppArmorException('Exec condition in change_profile rule does not start with /: %s' % str(execcond))
> +        else:
> +            raise AppArmorBug('Passed unknown object to ChangeProfileRule: %s' % str(execcond))
> 
> I know assigning self.execcond is still inside the elif block, but IMHO
> that's more readable than lots of 'not' condtions - especially with
> two or'ed conditions (which would become 'and not').

Yeah, just trying to not let the raised exceptions get too far away from
the tests that triggered them. Thanks.

> I can (and probably will) change that when we have some common code or
> even a class for handling paths.
> 
> 
> [ 01-add-ChangeProfileRule.diff ]

Acked-by: Steve Beattie <steve at nxnw.org>

-- 
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/20150528/95b73ee9/attachment.pgp>