[libvirt] Resubmission #2: [PATCH 2/3] sVirt AppArmor security driver

[Jamie Strandboge]

Attached is an updated patch_2_apparmor_driver_updated.patch and
patch_3_docs_updated.patch. Thanks for the review! These two patches
along with the previous patch_1_reenable-nonfile-labels.patch pass
syntax-check, make check (for the tests I added) and introduce no
regressions over the previous patch. See below for inline comments. In
addition to implenting your suggestions, the patch also now supports
<readonly/> for disks and I defensively quote the pid, monitor and
logfile. I also added ReserveSecurityLabel (a noop) along with stubs
for SetSecurityHostdevLabel() and RestoreSecurityHostdevLabel().

Jamie

On Wed, 30 Sep 2009, Daniel P. Berrange wrote:

> > +static int
> > +profile_status(const char *str, const int check_enforcing)
> > +{
> 
> Since you've already got yourself a 'rc' variable declared & initialized,
> it'd benice to replace the duplicated error paths:
> 
Done (along with others you mentioned).

> > +static int
> > +profile_status_file(const char *str)
> 
> This one should just be   virReportOOMError(NULL);
> 
Done (along with others you mentioned).

> > +static int
> > +load_profile(virConnectPtr conn, const char *profile, virDomainObjPtr vm,
> 
> No need to report an error when the virDomainDef* functions
> fail, since they'll have already done that for you.
> 
Done

> Casting away const multiple times here. THis can be avoided by
> changing 
> 
This is a favorite thing for me to do. :P Done (along with others you
mentioned).

> I find the out-of-order initialization of argv indicies here
> a little confusing. Even though it'd be duplicating a little,
> I think it'd be clearer to have
> 
Done

> I think I'd prefer to see this returning a string allocated on the heap,
> rather than the caller's stack. The whole method could thus be simplified
> down to just
> 
Done

> > +/* returns -1 on error or profile for libvirtd is unconfined, 0 if complain
> > + * mode and 1 if enforcing
> > + */
> > +static int
> > +use_apparmor(void)
> 
> Is this the best way to check if app armour is enabled on a  host ?
> It ought to be possible to use the app armour security driver for
> protecting guests regardless of whether the libvirtd daemon itself
> has a profile surely.
> 
Actually, this is required. Currently, an unconfined process may not
aa_change_profile(), which is why I am very careful here.

> > +if WITH_SECDRIVER_APPARMOR
> > +bin_PROGRAMS += virt-aa-helper
> 
> Since this program is only really intended for libvirt to call
> rather than general admin usage I think it'd be beter to list
> it as    libexec_PROGRAMS, so it ends up in /usr/libexec
> instead of /usr/bin
> 
> I think I'd actually have it in the src/security/ directory,
> since we usually keep libexec programs alongside the driver
> code that's using them, just have tools/ for end user programs
> 
Done and done.

> > +    if (STRNEQLEN(AA_PREFIX, uuid, strlen(AA_PREFIX)))
> > +        return -1;
> 
> This one can just be
> 
>       if (!STRPREFIX(uuid, AA_PREFIX))
>             return -1;
> 
Done

> How about just using  virUUIDParse to check UUID validity, such as
> 
>       char rawuuid[VIR_UUID_BUFLEN];
>       if (virUUIDParse(uuid + strlen(AA_PREFIX), rawuuid) < 0)
>           return -1;
>       return 0;
> 
Done

> FYI, there'a convenient libc function for checking a string for bad
> characters - strspn, you can use it in this way:
> 
>      if (strspn(name, bad) != strlen(name))
>          return -1;
> 
Ah, missed that one. I actually used strcspn() instead. Done


> > +static int
> > +valid_path(const char *path, const bool readonly)
> 
> More const issues. Rather than manually declaring the array
> size, and indexing manually its safer to initialize the
> array at time of declaration with all the strings.  I'd
> probably go for separate arrays for the read-write vs
> read-only difference
> 
Done

> > +static int
> > +vah_add_file(virBufferPtr buf, const char *path, const char *perms)
> > +
> > +    if (virBufferError(buf)) {
> > +         vah_error(NULL, 0, "failed to allocate file buffer");
> > +         rc = -1;
> > +    }
> 
> It is not really neeccesssar to check virBufferError here. The
> idea is that you can call virBufferVSprintf() as many times
> as you like in a row without any checking, and then only check
> virBufferError at the end when you've finishing writing to the
> buffer.
> 
Done

> > +static int
> > +get_files(vahControl * ctl)
> > +{
> > +    virBuffer uuid_buf = VIR_BUFFER_INITIALIZER;
> 
> Using virBuffer for a single printf here is a little overkill - the
> virAsprintf() function would be more than sufficient
> 
Yeah, at one point I was thinking of doing more here and forgot to go
back to virAsprintf(). Done.

> > +    /* needs patch to hostusb.h to work
> > +    for (i = 0; i < ctl->def->nhostdevs; i++)
> > +        if (ctl->def->hostdevs[i]) {
> > +            virDomainHostdevDefPtr hostdev = ctl->def->hostdevs[i];
> > +            if (hostdev->source.subsys.type ==
> > +                VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
> > +                usbDevice *usb = usbGetDevice(NULL,
> > +                           hostdev->source.subsys.u.usb.bus,
> > +                           hostdev->source.subsys.u.usb.device);
> > +                if (usb == NULL)
> > +                    continue;
> > +                rc = vah_add_file(&buf, usb->path, "rw");
> > +                usbFreeDevice(NULL, usb);
> 
> This is the bit of code that should be changed to use the
> usbDeviceFileIterate() method, since you can't assume there
> is only 1 path which is why we didn't make 'path' public.
> 
Done. virt-aa-helper now does this right, but overall attach-device with
a USB or PCI host device still does not work (need to figure out the best
way to get the hostdev XML to virt-aa-helper, as previously discussed).
Users can adjust the profile/abstraction in the meantime as desired.

> If you remove the virBufferError check from the vah_add_file
> calls, it could just go in here
> 
Done

-- 
Jamie Strandboge             | http://www.canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_2_apparmor_driver_updated.patch
Type: text/x-diff
Size: 69229 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091005/683928cb/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_3_docs_updated.patch
Type: text/x-diff
Size: 9157 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091005/683928cb/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091005/683928cb/attachment-0001.sig>