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

[John Johansen]

On 11/29/2014 11:19 AM, Christian Boltz wrote:
> 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 ;-)
> 
I'd say raise an exception