[libvirt] [PATCH] apparmor: QEMU bridge helper policy updates

[Corey Bryant]

Thanks for your comments.

One thing I should have pointed out before is that libvirt doesn't have 
support for the bridge helper yet.  I hard coded the qemu options in the 
domain XML to test this.  I'm not sure if that would prevent this patch 
from getting in or not.

On 03/12/2012 02:36 PM, Jamie Strandboge wrote:
> On Mon, 2012-03-12 at 09:13 -0400, Corey Bryant wrote:
>> This patch provides AppArmor policy updates for the QEMU bridge helper.
>> The QEMU bridge helper is a SUID executable exec'd by QEMU that drops
>> capabilities to CAP_NET_ADMIN and adds a tap device to a network
>> bridge. For more details on the helper, please refer to:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03562.html
>>
>> Signed-off-by: Corey Bryant<coreyb at linux.vnet.ibm.com>
>
> I've not used the helper personally, but the policy makes sense overall
> though. I do have a few questions:
>
>> +    capability setuid,
>> +    capability setgid,
>
> I'm assuming these are needed because qemu-bridge-helper drops
> privileges?
>

Yes, exactly.

>> +    capability setpcap,
>
> Can you explain why this capability is needed by qemu-bridge-helper?
>

This is required to modify the bounding capability set.  Here are the 
calls the helper uses to modify capabilities:

  capng_clear(CAPNG_SELECT_BOTH);
  capng_update(CAPNG_ADD,
               CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
  capng_change_id(getuid(), getgid(), CAPNG_CLEAR_BOUNDING);

>> +    network inet stream,
>
> I understood why net_admin was needed, but this one is less clear. Why
> does qemu-bridge-helper need this?
>

Good question.  I'm going to test without this and see if it's 
necessary.  I'm wondering if it's a subset of net_admin, and I may have 
added this before I added net_admin.

>> +    /etc/qemu/** r,
>
> I'm not familiar with this directory. What does qemu-bridge-helper need
> from this directory?
>

ACL config files can be configured to allow/disallow bridge access to 
specific users.  These files are read from /etc/qemu/.

>> +    @{PROC}/*/status r,
>
> Is it possible to use this instead:
> owner @{PROC}/*/status r,
>

I would imagine this is ok to update with owner.  I'll test it out and 
submit a v2 if there aren't any issues.

> Thanks!
>

Thank you!

-- 
Regards,
Corey