[libvirt] [PATCH] Refactor the security drivers to simplify usage
Daniel P. Berrange
berrange at redhat.com
Fri Jan 7 15:45:27 UTC 2011
On Thu, Jan 06, 2011 at 11:21:45AM -0700, Eric Blake wrote:
> On 01/06/2011 05:35 AM, Daniel P. Berrange wrote:
> > The current security driver usage requires horrible code like
> >
> > if (driver->securityDriver &&
> > driver->securityDriver->domainSetSecurityHostdevLabel &&
> > driver->securityDriver->domainSetSecurityHostdevLabel(driver->securityDriver,
> > vm, hostdev) < 0)
>
> This is a v2 of the earlier
> https://www.redhat.com/archives/libvir-list/2010-November/msg00984.html,
> but omits the rest of the 8-patch series that v1 was included with.
> That's okay, but I'm a bit curious on the progress of the rest of that
> series (in part because it involved adding virCommand handshaking, where
> I'm not sure if I need to lend a hand at getting that in) :)
This security driver patch is proving a total PITA to rebase with
people changes, so I want to get it merged ASAP independant of the
rest of the locking changes.
> > - /* No primary security driver wanted to be enabled: just setup
> > - * the DAC driver on its own */
> > - if (ret == -2) {
> > - qemud_drv->securityDriver = &qemuDACSecurityDriver;
> > - VIR_INFO0(_("No security driver available"));
> > + if (driver->privileged) {
> > + virSecurityManagerPtr dac = virSecurityManagerNewDAC(driver->user,
> > + driver->group,
> > + driver->allowDiskFormatProbing,
> > + driver->dynamicOwnership);
> > + if (!dac)
> > + return -1;
>
> Does this leak mgr?
>
> > +
> > + if (!(driver->securityManager = virSecurityManagerNewStack(mgr,
> > + dac)))
> > + return -1;
>
> Likewise.
Yep, both fixed.
> > @@ -1555,7 +1540,6 @@ qemudShutdown(void) {
> > VIR_FREE(qemu_driver->spicePassword);
> > VIR_FREE(qemu_driver->hugetlbfs_mount);
> > VIR_FREE(qemu_driver->hugepage_path);
> > - VIR_FREE(qemu_driver->securityDriverName);
> > VIR_FREE(qemu_driver->saveImageFormat);
> > VIR_FREE(qemu_driver->dumpImageFormat);
>
> Is there any state in a virSecurityManagerPtr that might necessitate a
> cleanup function (and even if there isn't right now, what happens if we
> extend virSecurityManager in the future), such that you are missing a
> call here to virSecurityManagerFree(qemu_driver->securityManager)?
Three was a missing call to virSecurityManagerFree
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * QEMU POSIX DAC security driver
>
> Do we still need the term QEMU here, or should it be dropped now that
> it's generic?
That's dropped.
>
> > +static int
> > +virSecurityDACSetOwnership(const char *path, int uid, int gid)
> > +{
> > + VIR_INFO("Setting DAC user and group on '%s' to '%d:%d'", path, uid, gid);
>
> %d and uid/gid are not compatible on cygwin; in util/util.c, we use %u,
> (unsigned int)uid for a portable alternative. (I'm not sure if this
> file would end up being compiled on cygwin, even though the old qemu
> version has been skipped to date). Multiple instances, but worth a
> separate cleanup patch, similar to commit c685993d7, so that this
> (already large) patch remains as a straight code motion with no hidden
> cleanup.
%d does work here, because uid/gid are both declared as ints,
not uid_t/gid_t.
> > +static int
> > +virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
> > + const char *path,
> > + size_t depth ATTRIBUTE_UNUSED,
> > + void *opaque)
> > +{
> > + virSecurityManagerPtr mgr = opaque;
> > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
>
> Rather than passing mgr as opaque, and recomputing priv each time
> through the loop, ...
There's no serious overhead there, so I prefer to pass the full object
around.
> > +
> > +void virSecurityDACSetUser(virSecurityManagerPtr mgr,
> > + uid_t user);
> > +void virSecurityDACSetGroup(virSecurityManagerPtr mgr,
> > + gid_t group);
> > +
> > +void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
> > + bool dynamic);
>
> Setters, but no getters. I guess that's okay for now.
There's no compelling need for any getters in this code.
> > diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> > new file mode 100644
> > index 0000000..7ab6e37
> > --- /dev/null
> > +++ b/src/security/security_manager.c
> > @@ -0,0 +1,291 @@
>
> No copyright header?
>
> > +virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary,
> > + virSecurityManagerPtr secondary)
> > +{
> > + virSecurityManagerPtr mgr =
> > + virSecurityManagerNewDriver(&virSecurityDriverStack,
> > + virSecurityManagerGetAllowDiskFormatProbing(primary));
>
> Should this be
> virSecurityManagerGetAllowDiskFormatProbing(primary) ||
> virSecurityManagerGetAllowDiskFormatProbing(secondary)
>
> or is the intent that creating a stack allows you to override probing
> permitted in the secondary with a primary that disables probing?
All drivers should have the same probe setting so it
only needs to check the primary driver.
> > +
> > + virSecurityStackSetPrimary(mgr, primary);
> > + virSecurityStackSetSecondary(mgr, secondary);
>
> You need an early exit if mgr is NULL, since virSecurityStackSetPrimary
> isn't too happy with a NULL mgr.
Yep, fixed.
>
> > +virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user,
> > + gid_t group,
> > + bool allowDiskFormatProbing,
> > + bool dynamicOwnership)
> > +{
> > + virSecurityManagerPtr mgr =
> > + virSecurityManagerNewDriver(&virSecurityDriverDAC,
> > + allowDiskFormatProbing);
> > +
> > + virSecurityDACSetUser(mgr, user);
>
> Likewise about needing an early exit if mgr is NULL.
>
> > +void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
> > +{
> > + return mgr + sizeof(mgr);
>
> Won't work. You meant to do:
>
> return (char *) mgr + sizeof(mgr);
Strange how it actually worked, but that must have been luck,
or perhaps I mistakenly didn't have the DAC driver active
when i tested it first.
> static const char *
> virSecurityStackGetModel(virSecurityManagerPtr mgr)
>
> > +static int
> > +virSecurityStackVerify(virSecurityManagerPtr mgr,
> > + virDomainDefPtr def)
> > +{
> > + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> > + int rc = 0;
> > +
> > + if (virSecurityManagerVerify(priv->primary, def) < 0)
> > + rc = -1;
> > +
> > +#if 0
> > + if (virSecurityManagerVerify(priv->secondary, def) < 0)
> > + rc = -1;
> > +#endif
>
> What happened here? The original code verified the secondary driver
> first, not second, and here, you aren't even verifying the secondary
> driver. Are you really fixing a bug in the original code? If so, can
> we separate the bug fix from the code motion into two commits?
I enabled the second verify even though none of the secondary
drivers implement it.
> > +static int
> > +virSecurityStackGenLabel(virSecurityManagerPtr mgr,
> > + virDomainObjPtr vm)
> > +{
> > + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> > + int rc = 0;
> > +
> > + if (virSecurityManagerGenLabel(priv->primary, vm) < 0)
> > + rc = -1;
> > +#if 0
> > + if (virSecurityManagerGenLabel(priv->secondary, vm) < 0)
> > + rc = -1;
> > +#endif
>
> Likewise. Here, it makes a bit more sense that you only need to
> generate one label to be shared among both stacks, rather than two. But
> what if the primary doesn't care about labels while the secondary does -
> shouldn't you have a fallback in that case?
Our architecture only allows for a single label, so we can't
allow secondary drivers to generate labels. We do actually
want to enable this in the future though. So I've added a
comment about it.
All the things you mention should be fixed in v3, along with
a few other minor bugs I discovered after more testing
Daniel
More information about the libvir-list
mailing list