On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> Now that we know what metadata lock manager user wishes to use we
> can load it when initializing security driver. This is achieved
> by adding new argument to virSecurityManagerNewDriver() and
> subsequently to all functions that end up calling it.
>
> The cfg.mk change is needed in order to allow lock_manager.h
> inclusion in security driver without 'syntax-check' complaining.
> This is safe thing to do as locking APIs will always exist (it's
> only backend implementation that changes). However, instead of
> allowing the include for all other drivers (like cpu, network,
> and so on) allow it only for security driver. This will still
> trigger the error if including from other drivers.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> cfg.mk | 4 +++-
> src/lxc/lxc_controller.c | 3 ++-
> src/lxc/lxc_driver.c | 2 +-
> src/qemu/qemu_driver.c | 3 +++
> src/security/security_manager.c | 22 ++++++++++++++++++++++
> src/security/security_manager.h | 2 ++
> tests/seclabeltest.c | 2 +-
> tests/securityselinuxlabeltest.c | 2 +-
> tests/securityselinuxtest.c | 2 +-
> tests/testutilsqemu.c | 2 +-
> 10 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index 609ae869c2..e0a7b5105a 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -787,8 +787,10 @@ sc_prohibit_cross_inclusion:
> case $$dir in \
> util/) safe="util";; \
> access/ | conf/) safe="($$dir|conf|util)";; \
> - cpu/| network/| node_device/| rpc/| security/| storage/) \
> + cpu/| network/| node_device/| rpc/| storage/) \
> safe="($$dir|util|conf|storage)";; \
> + security/) \
> + safe="($$dir|util|conf|storage|locking)";; \
> xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \
> *) safe="($$dir|$(mid_dirs)|util)";; \
> esac; \
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 4e84391bf5..5f957eb1f8 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -2625,7 +2625,8 @@ int main(int argc, char *argv[])
> ctrl->handshakeFd = handshakeFd;
>
> if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver,
> - LXC_DRIVER_NAME, 0)))
> + LXC_DRIVER_NAME,
> + "nop", 0)))
> goto cleanup;
>
> if (ctrl->def->seclabels) {
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 8867645cdc..93aa25e9e6 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -1532,7 +1532,7 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg)
> flags |= VIR_SECURITY_MANAGER_REQUIRE_CONFINED;
>
> virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName,
> - LXC_DRIVER_NAME, flags);
> + LXC_DRIVER_NAME, "nop", flags);
I think we should use NULL instead of "nop"? Keeping the default of
"nop" under the covers (so to speak). Similar for other callers...
> if (!mgr)
> goto error;
>
[...]
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 9f770d8c53..5c8370c159 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
[...]
> static virClassPtr virSecurityManagerClass;
> @@ -52,6 +55,9 @@ void virSecurityManagerDispose(void *obj)
>
> if (mgr->drv->close)
> mgr->drv->close(mgr);
Order/setup issue here mgr->drv-> cannot be assumed right now!
> +
> + virObjectUnref(mgr->lockPlugin);
> +
> VIR_FREE(mgr->privateData);
> }
>
> @@ -71,6 +77,7 @@ VIR_ONCE_GLOBAL_INIT(virSecurityManager);
> static virSecurityManagerPtr
> virSecurityManagerNewDriver(virSecurityDriverPtr drv,
> const char *virtDriver,
> + const char *lockManagerPluginName,
> unsigned int flags)
> {
> virSecurityManagerPtr mgr = NULL;
> @@ -90,6 +97,14 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
> if (!(mgr = virObjectLockableNew(virSecurityManagerClass)))
> goto error;
>
> + if (!lockManagerPluginName)
> + lockManagerPluginName = "nop";
> +
> + if (!(mgr->lockPlugin = virLockManagerPluginNew(lockManagerPluginName,
> + NULL, NULL, 0))) {
> + goto error;
> + }
> +
Want to watching things die spectacularly? Don't pass NULL, "nop", or
"lockd" here... For example, change securityselinuxlabeltest.c to pass
"foobar" and enjoy.
Problem is that mgr->drv-> and eventually mgr->fd == -1 is assumed in
virSecurityManagerDispose.
> mgr->drv = drv;
> mgr->flags = flags;
> mgr->virtDriver = virtDriver;
[...]
> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
> index 48fee7cd28..85797411eb 100644
> --- a/tests/securityselinuxlabeltest.c
> +++ b/tests/securityselinuxlabeltest.c
> @@ -349,7 +349,7 @@ mymain(void)
> if (!rc)
> return EXIT_AM_SKIP;
>
> - if (!(mgr = virSecurityManagerNew("selinux", "QEMU",
> + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", "nop",
Just for "completeness", why not pass "lockd" here?
Wonder if it's worth creating a false test that passes a non existent
image (foobar) just to ensure it fails properly so that someone doesn't
inadvertently undo what you'll need to change...
> VIR_SECURITY_MANAGER_DEFAULT_CONFINED |
> VIR_SECURITY_MANAGER_PRIVILEGED))) {
> VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n",
With the callers passing NULL instead of "nop" and cleaning up the
New/Dispose code properly -
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
