4 p.m.
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