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

[Daniel P. Berrange]

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