9:45 a.m.
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