On Mon, Oct 05, 2009 at 04:00:02PM -0500, Jamie Strandboge wrote:
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
THanks for addressing all those issues. ACK to this new patch Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|