[apparmor] [patch 11/11] utils: add simple parsing of multi-line rules

[Steve Beattie]

On Thu, Mar 06, 2014 at 10:10:16PM +0100, Christian Boltz wrote:
> Am Mittwoch, 5. März 2014 schrieb Steve Beattie:
> > D-Bus rules in particular seem to get written as multi-line rules.
> > This patch adds very simple hackish support for multiple lines.
> > Essentially, what it does is if the parsing of a line doesn't match
> > anything and falls all the way through, it saves the line and
> > prepends it to the next line that occurs in the profile, but *only*
> > if the line does not have a trailing comma to indicate the end of a
> > rule. If the trailing comma exists, then it assumes that it's a rule
> > that it doesn't understand and aborts.
> 
> Just for the records: this works for all rule types, not only for dbus 
> rules.

Correct.

> Is it intentional that RE_RULE_HAS_COMMA does not start with  ^  ?

Let's just say that RE_RULE_HAS_COMMA was a bit over-engineered. And
yeah, it should have been anchored at the beginning of the line.
I've updated the regex in the attached patch, as well as added a second
to detect trailing comments (that need to filtered). Hopefully, the way
I split them up is more readable.

> > +        # we're dealing with a multiline statement
> > +        if lastline:
> > +            line = '%s %s' % (lastline, line)
> 
> You can probably (untested ;-) also set "lastline = None" here (since 
> you added it to line, it isn't needed anymore). This also makes the 
> following >10 "lastline = None" superfluous.

Yeah, adjusted patch.

> > Index: b/utils/test/test-dbus_parse.py
> > ===================================================================
> > --- a/utils/test/test-dbus_parse.py
> > +++ b/utils/test/test-dbus_parse.py
> 
> Hmm, that's a strange filename for testing a regex that works for all 
> rule types ;-)

Heh, okay, hint taken, split out into a separate test script, which
exercises both new regexs. More tests can be added here for
other/additional test cases.
> 
> > +    def test_without_comma_curly_brace_03(self):
> > +        '''simple no comma check w/empty curly braces'''
> > +        self._check('member={} ', False)
> > +
> >  if __name__ == '__main__':
> >      unittest.main()
> 
> I'd merge the testcases into arrays, so that you have one array for 
> "with comma" and one for "without comma", and then loop over the arrays. 
> This should keep the code more readable, even if we add 50 additional 
> test rules.

I created an array, but I use each string in a 'no comma' and 'with
comma' situation, using %s replacement with either a comma or a space.
Kinda overkill, but works. I also used an array with generator for the
new split comment out regex.

> Speaking about additional test rules - please also add testcases for:
>
>     audit "/tmp/foo, # bar" 
> 
>     audit "/tmp/foo, # bar" rw,
> 
>     audit "/tmp/foo, # bar" rw, # comment

Good catches, I've added cases for the above.

>     member={Hello,AddMatch,RemoveMatch,
> 
>     dbus (r, w,

So... the former is invalid syntax under the parser (though it
is if you encapsulate the alternation in "s), while the latter is
accepted (in the context of additional text to make a complete rule
on additional lines). However, given that until this patch set, the
tools wouldn't parse multi-line rules *at all* and the complexity of
getting the above situations correct, at this point I'm inclined to
say that you probably shouldn't organize your rules like so, similar
to how we asserted you shouldn't use multi line rules if you wanted
to use the tools until now.

Anyway. Updated patch attached.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: utils-handle_multiline_rules_better.patch
Type: text/x-diff
Size: 9312 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140306/aa12eedc/attachment.patch>
-------------- 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/20140306/aa12eedc/attachment.pgp>