[apparmor] [patch] parser - add support for variable expansion in dbus rules

[Steve Beattie]

On Thu, Aug 29, 2013 at 10:45:02AM -0700, Seth Arnold wrote:
> On Thu, Aug 29, 2013 at 09:17:24AM -0700, Steve Beattie wrote:
> > Subject: parser - add support for variable expansion in dbus rules
> > Bug: https://bugs.launchpad.net/bugs/1218099
> > 
> > This patch adds support for expanding variables with dbus rules.
> > Specifically, they can expanded within the bus, name, path, member,
> > interface, and peer label fields.
> > 
> > Parser test cases and regression test cases are added as well.
> > 
> > Signed-off-by: Steve Beattie <sbeattie at ubuntu.com>
> 
> Acked-by: Seth Arnold <seth.arnold at canonical.com>
> 
> I've got some comments inline -- feel free to commit with or without
> any further changes. (The small 'ret' change isn't worth much effort
> one way or another, and the testing suggestion is large enough that it
> shouldn't block anything.)

Sorry, I hadn't seen your comments when I sent my v2 patch. The only
difference in that one from the v1 version that you reviewed is the
additional parser/tst/equality.sh tests.

> > +static int process_dbus_variables(struct dbus_entry *entry_list)
> > +{
> > +	int ret = TRUE, rc;

[SNIP]

> > +	return ret;
> > +}
> 
> The final 'return ret' is the only use of 'ret' aside from
> initialization. You might as well drop 'ret' and just return TRUE at
> the end directly.

Yes, sorry, this was a bit of monkey-see-monkey-do code. I think the
original idea around having the separate return value was to support
finding and reporting all the variable errors at once instead of
having the profile author iterate over fixing multiple errors. But
the existing code doesn't do that.

> These tests are a great start, but only one of them uses variables
> in two fields. I'd love to see one with all the fields -- though it
> would probably only tell us something new if we could compare the
> after-expansion with a known-good expected expansion.

Well, this is a bit of black-box knowledge, but if a variable fails to
get expanded, the parser fails with an error about alternation errors.
Which doesn't make the issue you raise with the coverage of these
tests any less valid.

I'd intended to have one or more of the added dbus_message regression
tests exercise variable expansion in most/all of the fields as well, but
overlooked it before sending the patch. I'll fix it.

The added equality tests in the v2 patch have a couple of tests that
exercise most or all of the possible fields and verify that they
generate the identical binary blob as what the expected expansion is.
However, I have a minor issue with the way that test script it setup
as it doesn't support any negative tests (that is, verifying that
different rules generate different binary blobs).

-- 
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: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130829/e6e098de/attachment.pgp>