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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list