[PATCH 2] patch_2_apparmor_driver.patch (updated and merged into one patch based on prior feedback): - Updates src/security.c for AppArmor - Adds security_apparmor.c, security_apparmor.h, virt-aa-helper.c and updates po/POTFILES.in. virt-aa-helper.c is a new binary which is used exclusively by the AppArmor security driver to manipulate AppArmor. - Adds tests for virt-aa-helper and the security driver. secaatest.c is identical to seclabeltest.c except it initializes the 'apparmor' driver instead of 'selinux'. These tests are integrated into 'make check' and pass. - Updates configure.in, Makefile.am, src/Makefile.am, tests/Makefile.am, and tools/Makefile.am for AppArmor. It is based on and should operate the same as the SELinux configuration.

> +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:

> +static int > +profile_status_file(const char *str) This one should just be virReportOOMError(NULL);

> +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.

Casting away const multiple times here. THis can be avoided by changing

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

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

> +/* 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.

> +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

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

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;

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;

> +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

> +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.

> +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

> + /* 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.

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

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

[PATCH 3] patch_3_docs.patch (Updated based on prior feedback): Updates docs/drvqemu.html.in for AppArmor and adds profile examples to examples/apparmor. Updated based on prior feedback.

On Fri, Sep 25, 2009 at 05:44:35PM -0500, Jamie Strandboge wrote: > > This simplifies adding new files. Part of this process involved adding > support for kernel, initrd, loader, serial, and console. I have commented > out code for hostdev (and pci would be easy enough), but it requires a > patch to to move the _usbDevice struct fully into hostusb.h. I figure > once the AppArmor driver is in, I can submit a patch for that. No, the usbDevice struct is intended to be private because we want the ability to easily change it without impacting callers. The idea is that there is not neccessarily just 1 single path associated with a USB device and thus 'path' should not be exposed to callers. Instead if you need process the 1-or-more paths associated with a USB device you should invoke the usbDeviceFileIterate() method and the callback you supply will be executing once for each path. The same pattern is there for PCI devices which have between 1 and 4 paths that need dealing with typically.

For virCapsPtr issues we should probably evaluate whether we can make the capabilities object passed to virDomainDefParse() optionally NULLable. The capabilities object is used to fill in missing pieces of metadata when getting an XML blob in from the client app. By the time we get into your virt-aa-helper, all this metadata ought to be present & correct, so in theory there should be no need for a virCapsPtr object.

Does the virt-aa-helper need the full domain XML when attaching or detaching a disk ? If not, then you could try an approach where you pass the full domain XML when initially starting the guest, and then only pass the domain UUID + the device XML when attaching or detaching a particular device on the fly.