[apparmor] [patch] add tests for aamode.py

[Christian Boltz]

Hello,

Am Samstag, 29. November 2014 schrieb Steve Beattie:
> On Fri, Nov 28, 2014 at 07:47:29PM +0100, Christian Boltz wrote:
> > Am Donnerstag, 27. November 2014 schrieb Steve Beattie:
> > > If we're going to raise an exception that nobody's going to check
> > > for, wuld it be better to raise AppArmorBug, as that seems more
> > > appropriate, that either there's a bug in our code or we got a
> > > dodgy
> > > log message with a weird permission mode. Plus we can attach
> > 
> > > information about why things went wrong. Patch follows:
> > So we replace an exception that nobody is going to check for with
> > another exception (with some additional details) that nobody is
> > going to check for? ;-)
> 
> Err? I thought we were going to at some point do some fancy
> logging/handling of AppArmorBug exceptions, no? So while in the
> short term, it doesn't make much difference except in the detailed
> reporting, I thought getting things set up for where you wanted to
> go in the future made sense.

You are taking me too serious ;-)

As you can see by the Acked-by, this was not an objection - but it was 
an easy chance for a joke, so I took the opportunity.

Having better error messages is always nice, so don't worry too much ;-)
(and yes, I also want the more detailed error reporting!)

> > > +    if ":" in user or ":" in other:
> > > +        raise AppArmorBug("After splitting %s, user (%s) or other
> > > (%s) contained ':' " % (mode, user, other))
> > 
> > This check makes aa-logprof slightly slower (~2% - 1:10 instead of
> > 1:09 runtime for 20 aa-logprof runs on a 4 MB audit.log).
> > 
> > Valid data might be worth that - OTOH, the function doesn't check if
> > user or other contain unknown letters, numbers or even special chars
> > (splitting "yes!!11!!::no§42" will work successfully, at least until
> > you try to use the result ;-)
> > 
> > So I'm undecided if adding this check really makes sense - given
> > that we only check for ":" and allow any other strange character,
> > it feels like a drop in the ocean ;-)
> 
> Well, do we check for sanity on permissions anywhere else that would
> detect if we got a wonky log entry? Somewhere we should ensure that we
> don't write out garbage to a profile, right?

split_log_mode() is only called by str_to_mode(), which then calls 
sub_str_to_mode() with both parts of the result. sub_str_to_mode() then 
break's the for loop when it hits an unknown char. This means it 
silently(!) ignores everything that follows after the unknown character, 
and returns only valid things it found before hitting the unknown 
character.

See for example
    def test_sub_str_to_mode_8(self):
        self.assertEqual(sub_str_to_mode('asdf42'), {'a'})


Now the question is if sub_str_to_mode shoud be non-silent and print a 
warning or raise an exception instead. (Given that valid_log_mode, if 
called, restricts the mode to LOG_MODE_RE, raising an exception 
shouldn't break anything.)

I'll wait for your opinion before writing a patch ;-)


Regards,

Christian Boltz
-- 
Die Bundesregierung hat beschlossen, daß alle Yast-Programme ab sofort
eine 48 Stunden Woche haben müssen. Bei dem was diese Programme an
Aufräumarbeit hinterlassen, kann das nur gut sein, um die
Arbeitslosenzahlen zu reduzieren. [Emil Stephan in suse-linux]