[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Refactor the security drivers to simplify usage



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]